From 9c12bcb8608393512992ed7af3d8a66ad43dbf1e Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sun, 6 Sep 2026 11:55:49 +0200 Subject: [PATCH 01/10] fix(ci): re-run every open PR's checks when main gains a new publication gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pull-request workflow runs only on that PR's own events, so every PR already open when a new job joins `CI - Required Checks` keeps the green it earned before that job existed. The branch rule keys on the check's name, so the stale run satisfies it and the PR can merge without the new gate ever running against it โ€” the way a stale plugin version or a hand-edited synced skill would reach consumers past the checks added to stop exactly that. Add a main-side workflow that re-triggers every open PR whenever `ci.yaml` changes, plus a manual dispatch with a dry run. Re-triggering is a close and immediate reopen: 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, and `reopened` is the only fresh `pull_request` event that leaves the head โ€” and any green review at it โ€” untouched. It runs under an App token because events produced with `GITHUB_TOKEN` start no workflow runs. The logic lives in a script with a hermetic self-test that stubs `gh`: twelve cases pin the close-before-reopen order, that no PR is ever left closed when a step fails mid-sequence, that an armed auto-merge is restored without one being armed that was not, and that an unparseable listing fails closed instead of reading as "no open PRs". Two ablations confirm the suite fires: neutralising the exit trap fails only the trap case, and arming auto-merge unconditionally fails only the auto-merge case. GitHub's own mechanism, `strict_required_status_checks_policy`, is declared org-wide and Observe-only, so enabling it is a maintainer decision across every repository rather than this one's to make; AGENTS.md records that alongside the workflow. Fixes #105 Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yaml | 8 + .github/workflows/recheck-open-prs.yaml | 67 ++++++++ AGENTS.md | 29 ++++ scripts/recheck-open-prs.sh | 178 ++++++++++++++++++++++ scripts/recheck-open-prs.test.sh | 193 ++++++++++++++++++++++++ 5 files changed, 475 insertions(+) create mode 100644 .github/workflows/recheck-open-prs.yaml create mode 100755 scripts/recheck-open-prs.sh create mode 100755 scripts/recheck-open-prs.test.sh 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..33946ce --- /dev/null +++ b/.github/workflows/recheck-open-prs.yaml @@ -0,0 +1,67 @@ +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 the CI definition changes on `main`, 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. + +on: + push: + branches: [main] + paths: + - .github/workflows/ci.yaml + 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, so the newest one does strictly more than the one it displaced. + group: recheck-open-prs + 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 + + - name: ๐Ÿ”‘ Generate GitHub App token + id: app-token + # Events produced with GITHUB_TOKEN do not start new workflow runs, so a reopen + # performed with it would be silent โ€” the same reason update-agent-skills.yaml mints an + # App token to open its PR. + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + client-id: ${{ vars.APP_CLIENT_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + permission-pull-requests: write + + - name: ๐Ÿ” Re-trigger open pull requests + env: + GH_TOKEN: ${{ steps.app-token.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..c9b41af 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,33 @@ 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: any push to +`main` touching `ci.yaml` re-triggers every open PR's checks, and it can be dispatched by hand +(with a `dry-run` input) after any other change that ought to be re-evaluated. 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. + +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 +without one ever being armed that was 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..6e33cab --- /dev/null +++ b/scripts/recheck-open-prs.sh @@ -0,0 +1,178 @@ +#!/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. +# +# 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. A PR is never left closed: the exit trap reopens +# anything this script closed and did not reopen. +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 +} +command -v jq > /dev/null 2>&1 || { + echo "recheck-open-prs: jq is required" >&2 + exit 2 +} + +# Records a PR from the moment it is closed until it is reopened. The trap is what makes a +# crash, a cancelled job, or an API failure mid-sequence safe: the window in which a PR is +# closed is the window in which its number sits in this file. +pending=$(mktemp) || exit 2 +# shellcheck disable=SC2329 # invoked indirectly, by the EXIT trap below +reopen_pending() { + local n + while IFS= read -r n; do + [ -n "$n" ] || continue + echo "recheck-open-prs: reopening #$n left closed by an interrupted run" >&2 + gh pr reopen "$n" --repo "$repo" > /dev/null 2>&1 || { + echo "::error::#$n could not be reopened; reopen it by hand" >&2 + } + done < "$pending" + rm -f "$pending" +} +trap reopen_pending EXIT + +if ! prs=$(gh pr list --repo "$repo" --state open --base "$base" --limit 100 \ + --json number,title,isDraft,autoMergeRequest); then + echo "recheck-open-prs: could not list open pull requests" >&2 + exit 2 +fi +# An empty listing and a failed one must not read alike: the command's own status is checked +# above, so an empty array here is a real "nothing open". +if ! count=$(printf '%s' "$prs" | jq -r 'length'); then + echo "recheck-open-prs: open pull request listing is not valid JSON" >&2 + exit 2 +fi + +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 automerge title; do + [ -n "$number" ] || continue + + if [ "$dry_run" -eq 1 ]; then + echo " would re-trigger #$number (auto-merge=$automerge) โ€” $title" + done_count=$((done_count + 1)) + continue + 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. + 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 + printf '%s\n' "$number" >> "$pending" + + 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 + # Reopened: drop it from the crash-recovery list. + if ! remaining=$(grep -v -x -- "$number" "$pending"); then + remaining="" + fi + printf '%s' "$remaining" > "$pending" + [ -z "$remaining" ] || printf '\n' >> "$pending" + + # Closing a PR clears an armed auto-merge request, so restore one that was armed. The + # repository allows squash only, so the method is not a guess. + if [ "$automerge" = "armed" ]; then + if ! gh pr merge "$number" --repo "$repo" --auto --squash > /dev/null; then + echo "::error::#$number was re-triggered but its auto-merge could not be re-armed" + failed=$((failed + 1)) + continue + fi + echo " re-triggered #$number and re-armed auto-merge โ€” $title" + else + echo " re-triggered #$number โ€” $title" + fi + done_count=$((done_count + 1)) +done < <(printf '%s' "$prs" | jq -r '.[]|[(.number|tostring), (if .autoMergeRequest == null then "none" else "armed" end), .title]|@tsv') + +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..cea16b1 --- /dev/null +++ b/scripts/recheck-open-prs.test.sh @@ -0,0 +1,193 @@ +#!/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. Each case asserts the property that makes the script safe to point +# at a live repository โ€” the ORDER of close and reopen, that a PR is never left closed, that an +# armed auto-merge is restored and an unarmed one is not created, and that a listing failure is +# distinguishable from an empty listing. +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 + +# Build a stub `gh` whose `pr list` returns $2 and whose mutating verbs append to a call log. +# $3 names a verb that must fail once, so the recovery paths are exercised for real rather than +# reasoned about. +make_gh() { + local dir="$1" listing="$2" fail_verb="${3:-}" + mkdir -p "$dir/bin" + printf '%s\n' "$listing" > "$dir/listing.json" + cat > "$dir/bin/gh" <> "\$log" +if [ -n "$fail_verb" ] && [ "\$2" = "$fail_verb" ]; then + exit 1 +fi +exit 0 +EOF + chmod +x "$dir/bin/gh" + : > "$dir/calls.log" +} + +run_script() { + local dir="$1" + shift + env PATH="$dir/bin:$PATH" "$SCRIPT" --repo owner/name "$@" 2>&1 +} + +# Same stubbed PATH, but every argument is the caller's โ€” for the cases that must pass a +# malformed `--repo` rather than the well-formed one run_script supplies. +run_raw() { + local dir="$1" + shift + env PATH="$dir/bin:$PATH" "$SCRIPT" "$@" 2>&1 +} + +# `grep -c` exits 1 on no match, so a `|| echo 0` fallback prints the count AND the fallback, +# yielding "0\n0" โ€” which every numeric comparison below would then reject. awk always exits 0. +calls() { awk 'END { print NR }' "$1/calls.log"; } + +TWO_PRS='[{"number":11,"title":"first","isDraft":false,"autoMergeRequest":null}, + {"number":22,"title":"second","isDraft":true,"autoMergeRequest":{"enabledAt":"2026-01-01T00:00:00Z"}}]' + +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 ] && [ "$(calls "$d")" -eq 0 ] && [[ $out == *"nothing to re-trigger"* ]]; then + ok "an empty listing exits 0 and mutates nothing" +else + bad "an empty listing exits 0 and mutates nothing" "exit $rc, $(calls "$d") call(s): $out" +fi + +# --- dry run ------------------------------------------------------------- +d="$WORK/dry" +make_gh "$d" "$TWO_PRS" +out=$(run_script "$d" --dry-run); rc=$? +if [ "$rc" -eq 0 ] && [ "$(calls "$d")" -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, $(calls "$d") call(s): $out" +fi + +# --- the happy path, and the ORDER that makes it safe -------------------- +d="$WORK/happy" +make_gh "$d" "$TWO_PRS" +out=$(run_script "$d"); rc=$? +log=$(cat "$d/calls.log") +if [ "$rc" -eq 0 ] && [[ $log == *"pr close 11"* ]] && [[ $log == *"pr reopen 11"* ]]; then + ok "each PR is closed and reopened" +else + bad "each PR is closed and reopened" "exit $rc" "$log" +fi +# Close BEFORE reopen for the same PR: the reverse order would leave it closed. +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 +# Auto-merge: restored only where it was armed. Arming one that was not is a merge the +# maintainer never asked for. +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 +# A draft is re-triggered like any other PR: a draft is exactly where a stale gate hides longest. +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 + +# --- a failing reopen must be reported, not swallowed -------------------- +d="$WORK/reopenfail" +make_gh "$d" "$TWO_PRS" reopen +out=$(run_script "$d"); rc=$? +if [ "$rc" -eq 1 ] && [[ $out == *"could not be reopened"* ]]; then + ok "a failing reopen exits nonzero and names the PR" +else + bad "a failing reopen exits nonzero and names the PR" "exit $rc: $out" +fi +# The trap is the safety net: every PR the script closed and could not reopen is retried on +# exit, so the closed window never outlives the run silently. +if [ "$(grep -c 'pr reopen 11' "$d/calls.log")" -ge 2 ]; then + ok "the exit trap retries a PR left closed" +else + bad "the exit trap retries a PR left closed" "$(cat "$d/calls.log")" +fi + +# --- a failing close leaves that PR untouched ---------------------------- +d="$WORK/closefail" +make_gh "$d" "$TWO_PRS" close +out=$(run_script "$d"); rc=$? +if [ "$rc" -eq 1 ] && [ "$(grep -c 'pr reopen' "$d/calls.log")" -eq 0 ]; then + ok "a PR that could not be closed is never reopened, and the run fails" +else + bad "a PR that could not be closed is never reopened, and the run fails" \ + "exit $rc" "$(cat "$d/calls.log")" +fi + +# --- a listing that is not JSON is an error, never an empty listing ------ +d="$WORK/badjson" +make_gh "$d" 'not json at all' +out=$(run_script "$d"); rc=$? +if [ "$rc" -eq 2 ] && [ "$(calls "$d")" -eq 0 ]; then + ok "an unparseable listing fails closed rather than reading as no open PRs" +else + bad "an unparseable listing fails closed rather than reading as no open PRs" \ + "exit $rc, $(calls "$d") call(s): $out" +fi + +echo "-----------------------------------------" +echo "recheck-open-prs.sh self-test: $pass passed, $fail failed" +[ "$fail" -eq 0 ] From c8ae96424aa80afb31bbe06578b8e0578a9e65a6 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sun, 6 Sep 2026 11:58:36 +0200 Subject: [PATCH 02/10] fix(scripts): silence the unreachable-trap warning on CI's older shellcheck too Local shellcheck 0.11 reports the trap-invoked helper as SC2329 on its declaration; the version CI installs reports every line of its body as SC2317. A directive naming only the local code passed here and failed there, so both are named. Co-Authored-By: Claude Opus 5 --- scripts/recheck-open-prs.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scripts/recheck-open-prs.sh b/scripts/recheck-open-prs.sh index 6e33cab..f58f075 100755 --- a/scripts/recheck-open-prs.sh +++ b/scripts/recheck-open-prs.sh @@ -89,7 +89,11 @@ command -v jq > /dev/null 2>&1 || { # crash, a cancelled job, or an API failure mid-sequence safe: the window in which a PR is # closed is the window in which its number sits in this file. pending=$(mktemp) || exit 2 -# shellcheck disable=SC2329 # invoked indirectly, by the EXIT trap below +# Invoked indirectly, by the EXIT trap below. Both codes are needed: shellcheck โ‰ฅ 0.11 reports +# the unused-looking function as SC2329 on this line, while older versions โ€” including the one CI +# installs โ€” report every line of its body as unreachable, SC2317. A directive naming only the +# local version's code passes here and fails there. +# shellcheck disable=SC2317,SC2329 reopen_pending() { local n while IFS= read -r n; do From f14980a92c42c60bcd8932e1e8696f8fe760428d Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sun, 6 Sep 2026 12:12:54 +0200 Subject: [PATCH 03/10] fix(scripts): sweep every open PR, and read auto-merge fresh per pull request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings, all real: - `gh pr list --limit 100` capped the sweep, so a repository with more than 100 open PRs left the rest on the pre-gate result โ€” this script's own failure mode one level down. Replaced with `gh api --paginate`, which walks every page. - The recovery record was written after the close, so a close that succeeded and then failed to be recorded left a PR the exit trap knew nothing about. It is now written before the close and removed if the close fails, which costs at most a harmless reopen of an already-open PR. - Auto-merge was read once for the whole sweep, so disabling it between the listing and a given PR's close would see it re-armed โ€” a merge nobody asked for. It is read per PR, immediately before closing, and a read that fails leaves that PR untouched rather than risking a silently dropped auto-merge. - The test's fault injection failed every matching call, so the reopen case proved only that a retry was attempted. It now fails the first call only, and the case asserts the retry succeeds. Writing the listing back with `printf '%s'` also dropped the final pull request, since command substitution strips the trailing newline โ€” caught by the new 101-PR case, which reported 100. Seventeen cases now, with three ablations partitioning cleanly: capping the sweep fails only the 101-PR case, never recording the close fails only the trap-retry case, and reading auto-merge from a stale snapshot fails all four auto-merge cases and nothing else. Co-Authored-By: Claude Opus 5 --- scripts/recheck-open-prs.sh | 93 ++++++++++----- scripts/recheck-open-prs.test.sh | 193 +++++++++++++++++++++++-------- 2 files changed, 210 insertions(+), 76 deletions(-) diff --git a/scripts/recheck-open-prs.sh b/scripts/recheck-open-prs.sh index f58f075..475490f 100755 --- a/scripts/recheck-open-prs.sh +++ b/scripts/recheck-open-prs.sh @@ -80,20 +80,17 @@ command -v gh > /dev/null 2>&1 || { echo "recheck-open-prs: gh is required" >&2 exit 2 } -command -v jq > /dev/null 2>&1 || { - echo "recheck-open-prs: jq is required" >&2 - exit 2 -} -# Records a PR from the moment it is closed until it is reopened. The trap is what makes a -# crash, a cancelled job, or an API failure mid-sequence safe: the window in which a PR is -# closed is the window in which its number sits in this file. +# Records a PR from the moment closing it is ATTEMPTED until it is reopened. The trap is what +# makes a crash, a cancelled job, or an API failure mid-sequence safe. The record is written +# before the close rather than after it, because a close that succeeds and then fails to be +# recorded would leave a closed PR the trap knows nothing about; a record whose close never +# happened costs only a harmless reopen of an already-open PR. pending=$(mktemp) || exit 2 -# Invoked indirectly, by the EXIT trap below. Both codes are needed: shellcheck โ‰ฅ 0.11 reports -# the unused-looking function as SC2329 on this line, while older versions โ€” including the one CI -# installs โ€” report every line of its body as unreachable, SC2317. A directive naming only the -# local version's code passes here and fails there. -# shellcheck disable=SC2317,SC2329 +# 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. reopen_pending() { local n while IFS= read -r n; do @@ -107,17 +104,35 @@ reopen_pending() { } trap reopen_pending EXIT -if ! prs=$(gh pr list --repo "$repo" --state open --base "$base" --limit 100 \ - --json number,title,isDraft,autoMergeRequest); then +# Drop $1 from the pending record. Rewritten wholesale rather than appended to, so the file is +# always the exact set of PRs currently closed by this run. +forget_pending() { + local keep + if ! keep=$(grep -v -x -- "$1" "$pending"); then + keep="" + fi + if [ -z "$keep" ]; then + : > "$pending" + else + printf '%s\n' "$keep" > "$pending" + fi +} + +# `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, so the sweep is complete +# however many are open. +# +# 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 "repos/${repo}/pulls?state=open&base=${base}&per_page=100" \ + --jq '.[]|[(.number|tostring), (.title // "")]|@tsv'); then echo "recheck-open-prs: could not list open pull requests" >&2 exit 2 fi -# An empty listing and a failed one must not read alike: the command's own status is checked -# above, so an empty array here is a real "nothing open". -if ! count=$(printf '%s' "$prs" | jq -r 'length'); then - echo "recheck-open-prs: open pull request listing is not valid JSON" >&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" @@ -129,35 +144,50 @@ echo "recheck-open-prs: re-triggering required checks on $count open PR(s) targe failed=0 done_count=0 -while IFS=$'\t' read -r number automerge title; do +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 (auto-merge=$automerge) โ€” $title" + 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. + if ! automerge=$(gh pr view "$number" --repo "$repo" --json autoMergeRequest \ + --jq 'if .autoMergeRequest == null then "none" else "armed" end'); then + echo "::error::#$number auto-merge state could not be read; left untouched" + failed=$((failed + 1)) + continue + 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. + printf '%s\n' "$number" >> "$pending" if ! gh pr close "$number" --repo "$repo" > /dev/null; then + # Never closed, so nothing to recover. + forget_pending "$number" echo "::error::#$number could not be closed; skipped without re-triggering" failed=$((failed + 1)) continue fi - printf '%s\n' "$number" >> "$pending" 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 - # Reopened: drop it from the crash-recovery list. - if ! remaining=$(grep -v -x -- "$number" "$pending"); then - remaining="" - fi - printf '%s' "$remaining" > "$pending" - [ -z "$remaining" ] || printf '\n' >> "$pending" + forget_pending "$number" # Closing a PR clears an armed auto-merge request, so restore one that was armed. The # repository allows squash only, so the method is not a guess. @@ -172,7 +202,10 @@ while IFS=$'\t' read -r number automerge title; do echo " re-triggered #$number โ€” $title" fi done_count=$((done_count + 1)) -done < <(printf '%s' "$prs" | jq -r '.[]|[(.number|tostring), (if .autoMergeRequest == null then "none" else "armed" end), .title]|@tsv') + # `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 diff --git a/scripts/recheck-open-prs.test.sh b/scripts/recheck-open-prs.test.sh index cea16b1..5d1b9ef 100755 --- a/scripts/recheck-open-prs.test.sh +++ b/scripts/recheck-open-prs.test.sh @@ -3,9 +3,9 @@ # # Hermetic: stubs `gh` on PATH and records every call, so nothing here reaches the network or # mutates a real pull request. Each case asserts the property that makes the script safe to point -# at a live repository โ€” the ORDER of close and reopen, that a PR is never left closed, that an -# armed auto-merge is restored and an unarmed one is not created, and that a listing failure is -# distinguishable from an empty listing. +# at a live repository โ€” the ORDER of close and reopen, that a PR is never left closed, that +# auto-merge is read fresh and restored only where it is armed NOW, that the sweep is complete +# past one API page, and that a malformed listing is distinguishable from an empty one. set -uo pipefail HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" @@ -28,24 +28,42 @@ bad() { WORK=$(mktemp -d) trap 'rm -rf "$WORK"' EXIT -# Build a stub `gh` whose `pr list` returns $2 and whose mutating verbs append to a call log. -# $3 names a verb that must fail once, so the recovery paths are exercised for real rather than -# reasoned about. +# Build a stub `gh`: +# $2 the TSV the paginated `api` listing emits (what the real --jq would produce) +# $3 a verb whose FIRST call fails; later calls succeed, so the exit trap's retry is exercised +# as a retry that can actually succeed rather than one that cannot +# $4 optional per-PR auto-merge states, "=armed|none ..."; default none +# `pr view` answers from a file rewritten per call, which is how the "disabled between the sweep +# and the close" case is expressed. make_gh() { - local dir="$1" listing="$2" fail_verb="${3:-}" - mkdir -p "$dir/bin" - printf '%s\n' "$listing" > "$dir/listing.json" + local dir="$1" listing="$2" fail_verb="${3:-}" automerge="${4:-}" + mkdir -p "$dir/bin" "$dir/state" + printf '%s' "$listing" > "$dir/listing.tsv" + local pair + for pair in $automerge; do + printf '%s' "${pair#*=}" > "$dir/state/am-${pair%%=*}" + done cat > "$dir/bin/gh" <> "\$log" + if [ -f "$dir/state/am-\$3" ]; then cat "$dir/state/am-\$3"; else printf 'none'; fi + printf '\n' + # A hook the caller can use to change state between the sweep and this PR's close. + [ -x "$dir/on-view" ] && "$dir/on-view" "\$3" exit 0 ;; esac printf '%s\n' "\$1 \$2 \$3" >> "\$log" -if [ -n "$fail_verb" ] && [ "\$2" = "$fail_verb" ]; then +if [ -n "$fail_verb" ] && [ "\$2" = "$fail_verb" ] && [ ! -f "$dir/state/failed-$fail_verb" ]; then + : > "$dir/state/failed-$fail_verb" exit 1 fi exit 0 @@ -60,27 +78,24 @@ run_script() { env PATH="$dir/bin:$PATH" "$SCRIPT" --repo owner/name "$@" 2>&1 } -# Same stubbed PATH, but every argument is the caller's โ€” for the cases that must pass a -# malformed `--repo` rather than the well-formed one run_script supplies. +# 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" "$SCRIPT" "$@" 2>&1 } -# `grep -c` exits 1 on no match, so a `|| echo 0` fallback prints the count AND the fallback, -# yielding "0\n0" โ€” which every numeric comparison below would then reject. awk always exits 0. calls() { awk 'END { print NR }' "$1/calls.log"; } -TWO_PRS='[{"number":11,"title":"first","isDraft":false,"autoMergeRequest":null}, - {"number":22,"title":"second","isDraft":true,"autoMergeRequest":{"enabledAt":"2026-01-01T00:00:00Z"}}]' +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=$? +make_gh "$d" '' +out=$(run_raw "$d" --repo) +rc=$? if [ "$rc" -eq 2 ]; then ok "a missing --repo value is a usage error" else @@ -88,8 +103,9 @@ else fi d="$WORK/slug" -make_gh "$d" '[]' -out=$(run_raw "$d" --repo not-a-slug); rc=$? +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 @@ -98,18 +114,33 @@ fi # --- nothing to do ------------------------------------------------------- d="$WORK/empty" -make_gh "$d" '[]' -out=$(run_script "$d"); rc=$? +make_gh "$d" '' +out=$(run_script "$d") +rc=$? if [ "$rc" -eq 0 ] && [ "$(calls "$d")" -eq 0 ] && [[ $out == *"nothing to re-trigger"* ]]; then ok "an empty listing exits 0 and mutates nothing" else bad "an empty listing exits 0 and mutates nothing" "exit $rc, $(calls "$d") call(s): $out" 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 ] && [ "$(calls "$d")" -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, $(calls "$d") call(s): $out" +fi + # --- dry run ------------------------------------------------------------- d="$WORK/dry" make_gh "$d" "$TWO_PRS" -out=$(run_script "$d" --dry-run); rc=$? +out=$(run_script "$d" --dry-run) +rc=$? if [ "$rc" -eq 0 ] && [ "$(calls "$d")" -eq 0 ] && [[ $out == *"would re-trigger #11"* ]] \ && [[ $out == *"would re-trigger #22"* ]]; then ok "--dry-run reports every PR and mutates nothing" @@ -119,8 +150,9 @@ fi # --- the happy path, and the ORDER that makes it safe -------------------- d="$WORK/happy" -make_gh "$d" "$TWO_PRS" -out=$(run_script "$d"); rc=$? +make_gh "$d" "$TWO_PRS" "" "22=armed" +out=$(run_script "$d") +rc=$? log=$(cat "$d/calls.log") if [ "$rc" -eq 0 ] && [[ $log == *"pr close 11"* ]] && [[ $log == *"pr reopen 11"* ]]; then ok "each PR is closed and reopened" @@ -134,8 +166,13 @@ if [ "$(grep -n 'pr close 11' "$d/calls.log" | cut -d: -f1)" -lt \ else bad "close precedes reopen for the same PR" "$log" fi -# Auto-merge: restored only where it was armed. Arming one that was not is a merge the -# maintainer never asked for. +# Auto-merge is read per PR, not once for the sweep โ€” that is what makes the state current. +if [ "$(grep -c 'pr view' "$d/calls.log")" -eq 2 ]; then + ok "auto-merge is read once per PR, immediately before closing it" +else + bad "auto-merge is read once per PR, immediately before closing it" "$log" +fi +# Restored only where it was armed. Arming one that was not is a merge nobody asked for. 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" @@ -149,43 +186,107 @@ else bad "a draft PR is re-triggered too" "$log" fi -# --- a failing reopen must be reported, not swallowed -------------------- +# --- auto-merge disabled between the sweep and the close ----------------- +# The window CodeRabbit named: a snapshot taken at listing time would re-arm an auto-merge the +# user turned off in between. Reading per PR is what closes it, so the state is changed after +# the listing and before this PR's own read. +d="$WORK/amrace" +make_gh "$d" "$TWO_PRS" "" "11=armed 22=armed" +cat > "$d/on-view" < "$d/state/am-22" +exit 0 +EOF +chmod +x "$d/on-view" +out=$(run_script "$d") +rc=$? +if [ "$rc" -eq 0 ] && [ "$(grep -c 'pr merge 11' "$d/calls.log")" -eq 1 ] \ + && [ "$(grep -c 'pr merge 22' "$d/calls.log")" -eq 0 ]; then + ok "auto-merge disabled after the listing is not re-armed" +else + bad "auto-merge disabled after the listing is not re-armed" "exit $rc" "$(cat "$d/calls.log")" +fi + +# --- the sweep is complete past one API page ----------------------------- +# `gh pr list --limit N` caps at N and silently drops the rest, which is this script's own +# failure mode one level down. 101 PRs is one more than that cap. +d="$WORK/many" +many=$(awk 'BEGIN { for (i = 1; i <= 101; i++) printf "%d\tpr %d\n", i, i }') +make_gh "$d" "$many" +out=$(run_script "$d" --dry-run) +rc=$? +if [ "$rc" -eq 0 ] && [[ $out == *"on 101 open PR(s)"* ]] && [[ $out == *"would re-trigger #101"* ]]; then + ok "every PR past the 100-item cap is swept" +else + bad "every PR past the 100-item cap is swept" "exit $rc: $(printf '%s' "$out" | tail -3)" +fi + +# --- a failing reopen must be reported, and the retry must WORK ---------- d="$WORK/reopenfail" make_gh "$d" "$TWO_PRS" reopen -out=$(run_script "$d"); rc=$? +out=$(run_script "$d") +rc=$? if [ "$rc" -eq 1 ] && [[ $out == *"could not be reopened"* ]]; then ok "a failing reopen exits nonzero and names the PR" else bad "a failing reopen exits nonzero and names the PR" "exit $rc: $out" fi -# The trap is the safety net: every PR the script closed and could not reopen is retried on -# exit, so the closed window never outlives the run silently. -if [ "$(grep -c 'pr reopen 11' "$d/calls.log")" -ge 2 ]; then - ok "the exit trap retries a PR left closed" +# The trap is the safety net, and the stub fails only the FIRST reopen โ€” so this asserts the +# retry actually succeeds, not merely that one was attempted. +if [ "$(grep -c 'pr reopen 11' "$d/calls.log")" -ge 2 ] \ + && [[ $out != *"could not be reopened; reopen it by hand"* ]]; then + ok "the exit trap retries a PR left closed, and the retry succeeds" else - bad "the exit trap retries a PR left closed" "$(cat "$d/calls.log")" + bad "the exit trap retries a PR left closed, and the retry succeeds" "$(cat "$d/calls.log")" "$out" fi # --- a failing close leaves that PR untouched ---------------------------- d="$WORK/closefail" make_gh "$d" "$TWO_PRS" close -out=$(run_script "$d"); rc=$? -if [ "$rc" -eq 1 ] && [ "$(grep -c 'pr reopen' "$d/calls.log")" -eq 0 ]; then +out=$(run_script "$d") +rc=$? +# #11's close fails, so it is never reopened and never recorded as closed; #22 proceeds normally. +if [ "$rc" -eq 1 ] && [ "$(grep -c 'pr reopen 11' "$d/calls.log")" -eq 0 ]; then ok "a PR that could not be closed is never reopened, and the run fails" else bad "a PR that could not be closed is never reopened, and the run fails" \ "exit $rc" "$(cat "$d/calls.log")" fi -# --- a listing that is not JSON is an error, never an empty listing ------ -d="$WORK/badjson" -make_gh "$d" 'not json at all' -out=$(run_script "$d"); rc=$? -if [ "$rc" -eq 2 ] && [ "$(calls "$d")" -eq 0 ]; then - ok "an unparseable listing fails closed rather than reading as no open PRs" +# --- an unreadable auto-merge state leaves the PR untouched -------------- +d="$WORK/amfail" +make_gh "$d" "$TWO_PRS" +cat > "$d/bin/gh" <> "\$log"; exit 1 ;; +esac +printf '%s\n' "\$1 \$2 \$3" >> "\$log" +exit 0 +EOF +chmod +x "$d/bin/gh" +out=$(run_script "$d") +rc=$? +if [ "$rc" -eq 1 ] && [ "$(grep -c 'pr close' "$d/calls.log")" -eq 0 ] \ + && [[ $out == *"auto-merge state could not be read"* ]]; then + ok "a PR whose auto-merge state cannot be read is left untouched" else - bad "an unparseable listing fails closed rather than reading as no open PRs" \ - "exit $rc, $(calls "$d") call(s): $out" + bad "a PR whose auto-merge state cannot be read is left untouched" \ + "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 "-----------------------------------------" From 4a0d62a1236cbec5b4d96442282d100e7d33f6b9 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sun, 6 Sep 2026 12:29:56 +0200 Subject: [PATCH 04/10] fix(scripts): settle recovery from real state, and stop relying on a capped path filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five review findings, all real: - A close can be applied and still report failure. Dropping the recovery record on a nonzero exit therefore left a genuinely closed PR with nothing tracking it โ€” contradicting this script's own invariant. The record now survives a failed close, and the exit trap decides from the pull request's actual state, reopening anything not OPEN or MERGED and treating an unreadable state as reason to reopen rather than reason to assume. - Once a close has cleared an auto-merge request, a later run cannot tell the PR ever had one, so a transient re-arm failure lost it permanently. The obligation now survives in the state directory and the trap retries it. - Re-arming with `--squash` alone discarded a squash subject or body someone chose. The commit metadata is captured before the close and restored with it. - The workflow's `paths:` filter is capped at 300 files, so a large sync could change `ci.yaml` without triggering the recheck โ€” the failure mode this workflow exists to prevent, hidden behind its own trigger. It now runs on every push to `main` and compares the pushed range itself, which has no cap, exiting early when the CI definition did not move. - The branch reached the API spliced into a query string, so a legal name containing `&` or `#` would select a different set of pull requests. It is passed as a GET field. Twenty-one cases now, against a stub that keeps real per-PR state, so they assert what is left behind rather than which calls were made. Three ablations partition cleanly: assuming a failed close did not apply fails only the ambiguous-close case, dropping the re-arm obligation fails only the transient-re-arm case, and re-arming without the captured metadata fails only the custom subject and body case. Co-Authored-By: Claude Opus 5 --- .github/workflows/recheck-open-prs.yaml | 38 +++- scripts/recheck-open-prs.sh | 128 +++++++----- scripts/recheck-open-prs.test.sh | 247 ++++++++++++++++-------- 3 files changed, 288 insertions(+), 125 deletions(-) diff --git a/.github/workflows/recheck-open-prs.yaml b/.github/workflows/recheck-open-prs.yaml index 33946ce..1b48f5f 100644 --- a/.github/workflows/recheck-open-prs.yaml +++ b/.github/workflows/recheck-open-prs.yaml @@ -14,10 +14,13 @@ name: ๐Ÿ” Recheck open PRs # resolves a fresh merge ref, and why it needs an App token. on: + # Deliberately NOT a `paths:` filter. GitHub caps a path filter's diff at 300 files, so a merge + # that changes more than that โ€” a large skill sync, a generated-content update โ€” can change + # `ci.yaml` without the filter seeing it, leaving every open PR on its stale result precisely + # when a gate moved. The job compares the pushed range itself, which has no such cap, and exits + # early when the CI definition did not change. push: branches: [main] - paths: - - .github/workflows/ci.yaml workflow_dispatch: inputs: dry-run: @@ -45,10 +48,40 @@ jobs: - name: ๐Ÿ“„ Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: + # The CI-definition check below diffs the pushed range, so both endpoints must be in + # the local history. + fetch-depth: 0 persist-credentials: false + - name: ๐Ÿ” Did the CI definition change? + id: gate + env: + BEFORE: ${{ github.event.before }} + AFTER: ${{ github.sha }} + run: | + # A manual dispatch always runs: it exists to force a sweep. + if [ "${GITHUB_EVENT_NAME}" != "push" ]; then + echo "changed=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + # A missing or unreachable `before` (a new branch, a force push, a squashed history) + # leaves the range unknowable. Sweep rather than skip: a needless recheck costs a CI + # run, a missed one leaves a gate unapplied. + if [ -z "${BEFORE}" ] || ! git cat-file -e "${BEFORE}^{commit}" 2> /dev/null; then + echo "No usable before-commit for this push โ€” sweeping." + echo "changed=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + if git diff --name-only "${BEFORE}" "${AFTER}" | grep -qx '.github/workflows/ci.yaml'; then + echo "changed=true" >> "$GITHUB_OUTPUT" + else + echo "The CI definition did not change in this push โ€” nothing to recheck." + echo "changed=false" >> "$GITHUB_OUTPUT" + fi + - name: ๐Ÿ”‘ Generate GitHub App token id: app-token + if: steps.gate.outputs.changed == 'true' # Events produced with GITHUB_TOKEN do not start new workflow runs, so a reopen # performed with it would be silent โ€” the same reason update-agent-skills.yaml mints an # App token to open its PR. @@ -59,6 +92,7 @@ jobs: permission-pull-requests: write - name: ๐Ÿ” Re-trigger open pull requests + if: steps.gate.outputs.changed == 'true' env: GH_TOKEN: ${{ steps.app-token.outputs.token }} DRY_RUN: ${{ inputs.dry-run && '--dry-run' || '' }} diff --git a/scripts/recheck-open-prs.sh b/scripts/recheck-open-prs.sh index 475490f..1e9eddf 100755 --- a/scripts/recheck-open-prs.sh +++ b/scripts/recheck-open-prs.sh @@ -27,13 +27,18 @@ # 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. A PR is never left closed: the exit trap reopens -# anything this script closed and did not reopen. +# be, 2 on a usage or environment error. set -uo pipefail usage() { @@ -81,52 +86,73 @@ command -v gh > /dev/null 2>&1 || { exit 2 } -# Records a PR from the moment closing it is ATTEMPTED until it is reopened. The trap is what -# makes a crash, a cancelled job, or an API failure mid-sequence safe. The record is written -# before the close rather than after it, because a close that succeeds and then fails to be -# recorded would leave a closed PR the trap knows nothing about; a record whose close never -# happened costs only a harmless reopen of an already-open PR. -pending=$(mktemp) || exit 2 +state=$(mktemp -d) || exit 2 +mkdir -p "$state/closed" "$state/rearm" || exit 2 + +# 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. -reopen_pending() { - local n - while IFS= read -r n; do - [ -n "$n" ] || continue - echo "recheck-open-prs: reopening #$n left closed by an interrupted run" >&2 - gh pr reopen "$n" --repo "$repo" > /dev/null 2>&1 || { - echo "::error::#$n could not be reopened; reopen it by hand" >&2 - } - done < "$pending" - rm -f "$pending" +settle() { + local f n st headline body + 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 + headline=$(cat "$state/rearm/$n/headline" 2> /dev/null) || headline="" + body=$(cat "$state/rearm/$n/body" 2> /dev/null) || body="" + echo "recheck-open-prs: restoring auto-merge on #$n" >&2 + rearm "$n" "$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" } -trap reopen_pending EXIT - -# Drop $1 from the pending record. Rewritten wholesale rather than appended to, so the file is -# always the exact set of PRs currently closed by this run. -forget_pending() { - local keep - if ! keep=$(grep -v -x -- "$1" "$pending"); then - keep="" - fi - if [ -z "$keep" ]; then - : > "$pending" - else - printf '%s\n' "$keep" > "$pending" - fi + +# Re-arm auto-merge, preserving the commit metadata the request carried. Recreating it with +# defaults would silently discard a squash subject or body someone chose deliberately. +rearm() { + local n=$1 headline=$2 body=$3 + set -- "$n" --repo "$repo" --auto --squash + [ -z "$headline" ] || set -- "$@" --subject "$headline" + [ -z "$body" ] || set -- "$@" --body "$body" + 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, so the sweep is complete -# however many are open. +# 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 "repos/${repo}/pulls?state=open&base=${base}&per_page=100" \ +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 @@ -171,12 +197,24 @@ while IFS=$'\t' read -r number title; do continue fi + if [ "$automerge" = "armed" ]; then + # Capture the commit metadata before the close clears the request, so the restore can put + # back what was there instead of a default message. + mkdir -p "$state/rearm/$number" + gh pr view "$number" --repo "$repo" --json autoMergeRequest \ + --jq '.autoMergeRequest.commitHeadline // ""' > "$state/rearm/$number/headline" 2> /dev/null \ + || : > "$state/rearm/$number/headline" + gh pr view "$number" --repo "$repo" --json autoMergeRequest \ + --jq '.autoMergeRequest.commitBody // ""' > "$state/rearm/$number/body" 2> /dev/null \ + || : > "$state/rearm/$number/body" + 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. - printf '%s\n' "$number" >> "$pending" + # 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 - # Never closed, so nothing to recover. - forget_pending "$number" echo "::error::#$number could not be closed; skipped without re-triggering" failed=$((failed + 1)) continue @@ -187,16 +225,20 @@ while IFS=$'\t' read -r number title; do failed=$((failed + 1)) continue fi - forget_pending "$number" + rm -f "$state/closed/$number" - # Closing a PR clears an armed auto-merge request, so restore one that was armed. The - # repository allows squash only, so the method is not a guess. if [ "$automerge" = "armed" ]; then - if ! gh pr merge "$number" --repo "$repo" --auto --squash > /dev/null; then + headline=$(cat "$state/rearm/$number/headline" 2> /dev/null) || headline="" + body=$(cat "$state/rearm/$number/body" 2> /dev/null) || body="" + if ! rearm "$number" "$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" diff --git a/scripts/recheck-open-prs.test.sh b/scripts/recheck-open-prs.test.sh index 5d1b9ef..fcfae25 100755 --- a/scripts/recheck-open-prs.test.sh +++ b/scripts/recheck-open-prs.test.sh @@ -2,10 +2,9 @@ # 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. Each case asserts the property that makes the script safe to point -# at a live repository โ€” the ORDER of close and reopen, that a PR is never left closed, that -# auto-merge is read fresh and restored only where it is armed NOW, that the sweep is complete -# past one API page, and that a malformed listing is distinguishable from an empty one. +# 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)" @@ -28,44 +27,94 @@ bad() { WORK=$(mktemp -d) trap 'rm -rf "$WORK"' EXIT -# Build a stub `gh`: -# $2 the TSV the paginated `api` listing emits (what the real --jq would produce) -# $3 a verb whose FIRST call fails; later calls succeed, so the exit trap's retry is exercised -# as a retry that can actually succeed rather than one that cannot -# $4 optional per-PR auto-merge states, "=armed|none ..."; default none -# `pr view` answers from a file rewritten per call, which is how the "disabled between the sweep -# and the close" case is expressed. +# 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:-}" automerge="${4:-}" - mkdir -p "$dir/bin" "$dir/state" + local dir="$1" listing="$2" fail_verb="${3:-}" armed="${4:-}" + mkdir -p "$dir/bin" "$dir/db" printf '%s' "$listing" > "$dir/listing.tsv" - local pair - for pair in $automerge; do - printf '%s' "${pair#*=}" > "$dir/state/am-${pair%%=*}" + 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 "$dir/listing-fails" ] && exit 1 + cat "$dir/listing.tsv" + exit 0 +fi + +n=\$3 +case "\$verb" in + "pr view") + field="" + for a in "\$@"; do [ "\$prev" = "--json" ] 2>/dev/null && field=\$a; prev=\$a; done + printf '%s\n' "pr view \$n \$field" >> "\$log" + [ -f "\$db/viewfail" ] && exit 1 + case "\$field" in + state) + if [ -f "\$db/closed-\$n" ]; then printf 'CLOSED\n'; else printf 'OPEN\n'; fi + ;; + autoMergeRequest) + # Which projection is asked for is inferred from the --jq expression. + case "\$*" in + *commitHeadline*) cat "\$db/headline-\$n" 2>/dev/null; printf '\n' ;; + *commitBody*) cat "\$db/body-\$n" 2>/dev/null; printf '\n' ;; + *) if [ -f "\$db/am-\$n" ]; then printf 'armed\n'; else printf 'none\n'; fi ;; + esac + ;; + 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 view") - printf '%s\n' "pr view \$3" >> "\$log" - if [ -f "$dir/state/am-\$3" ]; then cat "$dir/state/am-\$3"; else printf 'none'; fi - printf '\n' - # A hook the caller can use to change state between the sweep and this PR's close. - [ -x "$dir/on-view" ] && "$dir/on-view" "\$3" + "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" + 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' "\$1 \$2 \$3" >> "\$log" -if [ -n "$fail_verb" ] && [ "\$2" = "$fail_verb" ] && [ ! -f "$dir/state/failed-$fail_verb" ]; then - : > "$dir/state/failed-$fail_verb" - exit 1 -fi +printf '%s\n' "\$verb \$n" >> "\$log" exit 0 EOF chmod +x "$dir/bin/gh" @@ -86,6 +135,16 @@ run_raw() { } 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' @@ -117,10 +176,11 @@ d="$WORK/empty" make_gh "$d" '' out=$(run_script "$d") rc=$? -if [ "$rc" -eq 0 ] && [ "$(calls "$d")" -eq 0 ] && [[ $out == *"nothing to re-trigger"* ]]; then +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, $(calls "$d") call(s): $out" + 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 -------------------------------- @@ -129,11 +189,25 @@ make_gh "$d" "$TWO_PRS" : > "$d/listing-fails" out=$(run_script "$d") rc=$? -if [ "$rc" -eq 2 ] && [ "$(calls "$d")" -eq 0 ]; then +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, $(calls "$d") call(s): $out" + 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 ------------------------------------------------------------- @@ -141,45 +215,49 @@ d="$WORK/dry" make_gh "$d" "$TWO_PRS" out=$(run_script "$d" --dry-run) rc=$? -if [ "$rc" -eq 0 ] && [ "$(calls "$d")" -eq 0 ] && [[ $out == *"would re-trigger #11"* ]] \ - && [[ $out == *"would re-trigger #22"* ]]; then +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, $(calls "$d") call(s): $out" + 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=armed" +make_gh "$d" "$TWO_PRS" "" "22" out=$(run_script "$d") rc=$? log=$(cat "$d/calls.log") -if [ "$rc" -eq 0 ] && [[ $log == *"pr close 11"* ]] && [[ $log == *"pr reopen 11"* ]]; then - ok "each PR is closed and reopened" +if [ "$rc" -eq 0 ] && [ "$(left_closed "$d")" -eq 0 ]; then + ok "every PR is left open" else - bad "each PR is closed and reopened" "exit $rc" "$log" + bad "every PR is left open" "exit $rc" "$log" fi -# Close BEFORE reopen for the same PR: the reverse order would leave it closed. 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 -# Auto-merge is read per PR, not once for the sweep โ€” that is what makes the state current. -if [ "$(grep -c 'pr view' "$d/calls.log")" -eq 2 ]; then - ok "auto-merge is read once per PR, immediately before closing it" +if [ "$(grep -c 'pr view 11 autoMergeRequest' "$d/calls.log")" -ge 1 ] \ + && [ "$(grep -c 'pr view 22 autoMergeRequest' "$d/calls.log")" -ge 1 ]; then + ok "auto-merge is read per PR, immediately before closing it" else - bad "auto-merge is read once per PR, immediately before closing it" "$log" + bad "auto-merge is read per PR, immediately before closing it" "$log" fi -# Restored only where it was armed. Arming one that was not is a merge nobody asked for. 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 -# A draft is re-triggered like any other PR: a draft is exactly where a stale gate hides longest. +# 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 @@ -187,15 +265,12 @@ else fi # --- auto-merge disabled between the sweep and the close ----------------- -# The window CodeRabbit named: a snapshot taken at listing time would re-arm an auto-merge the -# user turned off in between. Reading per PR is what closes it, so the state is changed after -# the listing and before this PR's own read. d="$WORK/amrace" -make_gh "$d" "$TWO_PRS" "" "11=armed 22=armed" +make_gh "$d" "$TWO_PRS" "" "11 22" cat > "$d/on-view" < "$d/state/am-22" +[ "\$1" = "11" ] && rm -f "$d/db/am-22" exit 0 EOF chmod +x "$d/on-view" @@ -209,8 +284,6 @@ else fi # --- the sweep is complete past one API page ----------------------------- -# `gh pr list --limit N` caps at N and silently drops the rest, which is this script's own -# failure mode one level down. 101 PRs is one more than that cap. d="$WORK/many" many=$(awk 'BEGIN { for (i = 1; i <= 101; i++) printf "%d\tpr %d\n", i, i }') make_gh "$d" "$many" @@ -232,42 +305,56 @@ if [ "$rc" -eq 1 ] && [[ $out == *"could not be reopened"* ]]; then else bad "a failing reopen exits nonzero and names the PR" "exit $rc: $out" fi -# The trap is the safety net, and the stub fails only the FIRST reopen โ€” so this asserts the -# retry actually succeeds, not merely that one was attempted. -if [ "$(grep -c 'pr reopen 11' "$d/calls.log")" -ge 2 ] \ - && [[ $out != *"could not be reopened; reopen it by hand"* ]]; then - ok "the exit trap retries a PR left closed, and the retry succeeds" +if [ "$(left_closed "$d")" -eq 0 ] && [[ $out != *"reopen it by hand"* ]]; then + ok "the exit trap reopens a PR left closed, and the retry succeeds" +else + bad "the exit trap reopens a PR left closed, and the retry succeeds" \ + "$(left_closed "$d") still closed" "$out" +fi + +# --- a close that was APPLIED but reported failure ----------------------- +# The dangerous shape: a lost response or a timeout. Dropping the record on a nonzero exit would +# leave the PR closed with nothing tracking it, so the trap must decide from the real state. +d="$WORK/closeambiguous" +make_gh "$d" "$TWO_PRS" close-applied +out=$(run_script "$d") +rc=$? +if [ "$(left_closed "$d")" -eq 0 ]; then + ok "a close that reported failure but was applied is still reopened" else - bad "the exit trap retries a PR left closed, and the retry succeeds" "$(cat "$d/calls.log")" "$out" + bad "a close that reported failure but was applied is still reopened" \ + "exit $rc" "$(ls -1 "$d/db")" "$out" fi -# --- a failing close leaves that PR untouched ---------------------------- +# --- a failing close that really did not apply --------------------------- d="$WORK/closefail" make_gh "$d" "$TWO_PRS" close out=$(run_script "$d") rc=$? -# #11's close fails, so it is never reopened and never recorded as closed; #22 proceeds normally. -if [ "$rc" -eq 1 ] && [ "$(grep -c 'pr reopen 11' "$d/calls.log")" -eq 0 ]; then - ok "a PR that could not be closed is never reopened, and the run fails" +if [ "$rc" -eq 1 ] && [ "$(left_closed "$d")" -eq 0 ]; then + ok "a PR whose close failed is left open, and the run fails" +else + bad "a PR whose close failed is left open, and the run fails" "exit $rc" "$(cat "$d/calls.log")" +fi + +# --- a transient re-arm failure is recovered, not lost ------------------- +# Once the close has cleared the request, a later run cannot tell the PR ever had auto-merge +# armed โ€” so if this run drops the obligation, it is gone for good. +d="$WORK/mergefail" +make_gh "$d" "$TWO_PRS" merge "22" +out=$(run_script "$d") +rc=$? +if [ "$rc" -eq 1 ] && [ "$(armed_count "$d")" -eq 1 ] && [[ $out != *"re-arm it by hand"* ]]; then + ok "a transient re-arm failure is retried by the exit trap and restored" else - bad "a PR that could not be closed is never reopened, and the run fails" \ - "exit $rc" "$(cat "$d/calls.log")" + bad "a transient re-arm failure is retried by the exit trap and restored" \ + "exit $rc, armed=$(armed_count "$d")" "$out" fi # --- an unreadable auto-merge state leaves the PR untouched -------------- d="$WORK/amfail" make_gh "$d" "$TWO_PRS" -cat > "$d/bin/gh" <> "\$log"; exit 1 ;; -esac -printf '%s\n' "\$1 \$2 \$3" >> "\$log" -exit 0 -EOF -chmod +x "$d/bin/gh" +: > "$d/db/viewfail" out=$(run_script "$d") rc=$? if [ "$rc" -eq 1 ] && [ "$(grep -c 'pr close' "$d/calls.log")" -eq 0 ] \ From 0dc09cc48adf9d188262759647acdc763b11baf5 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sun, 6 Sep 2026 12:45:29 +0200 Subject: [PATCH 05/10] fix(ci): sweep on every main push, and restore auto-merge exactly as it was MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six review findings, all real. Three concerned deciding *whether* a gate changed, and together they show that decision cannot be made correctly here, so it is no longer made: - A pending gate-changing push coalesced away by the concurrency group is replaced by a later push whose own range shows no gate change, so 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, so adding a rejection there changes what the required check accepts while `ci.yaml` is untouched. - (The previous round already showed a `paths:` filter is capped at 300 files.) The workflow now sweeps on every push to `main`. The cost is bounded and visible, 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. The other three: - The App token requested pull-requests write only, but `gh pr merge --auto` also needs contents write. Close and reopen would have succeeded while every re-arm failed, so a PR that arrived with auto-merge armed would have been left without it. - Re-arming always used `--squash`, changing the merge behaviour of a request armed as a merge commit or a rebase. The strategy is captured and restored, and the commit-message flags now accompany a merge or squash only, since a rebase carries no message. - The per-PR reads were split, so a transient failure on a later one was indistinguishable from "no custom metadata" and would have restored GitHub's default message. State, strategy and metadata now come from one response, and a failed read leaves the PR untouched โ€” which also closes a second hole: reading auto-merge alone succeeds for a closed PR, so a PR the maintainer closed after the listing would have been reopened, reversing a deliberate act. Twenty-four cases; five ablations partition cleanly โ€” hardcoding the strategy fails only the two strategy cases, and dropping the open-state check fails only the closed-after-listing case. Co-Authored-By: Claude Opus 5 --- .github/workflows/recheck-open-prs.yaml | 71 ++++++++----------- scripts/recheck-open-prs.sh | 70 ++++++++++++------ scripts/recheck-open-prs.test.sh | 94 ++++++++++++++++++++----- 3 files changed, 154 insertions(+), 81 deletions(-) diff --git a/.github/workflows/recheck-open-prs.yaml b/.github/workflows/recheck-open-prs.yaml index 1b48f5f..7bb4b86 100644 --- a/.github/workflows/recheck-open-prs.yaml +++ b/.github/workflows/recheck-open-prs.yaml @@ -9,16 +9,26 @@ name: ๐Ÿ” Recheck open PRs # 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 the CI definition changes on `main`, 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. +# 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: - # Deliberately NOT a `paths:` filter. GitHub caps a path filter's diff at 300 files, so a merge - # that changes more than that โ€” a large skill sync, a generated-content update โ€” can change - # `ci.yaml` without the filter seeing it, leaving every open PR on its stale result precisely - # when a gate moved. The job compares the pushed range itself, which has no such cap, and exits - # early when the CI definition did not change. push: branches: [main] workflow_dispatch: @@ -32,7 +42,7 @@ 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, so the newest one does strictly more than the one it displaced. + # PR unconditionally, so the newest one does strictly more than the one it displaced. group: recheck-open-prs cancel-in-progress: false @@ -48,51 +58,26 @@ jobs: - name: ๐Ÿ“„ Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: - # The CI-definition check below diffs the pushed range, so both endpoints must be in - # the local history. - fetch-depth: 0 persist-credentials: false - - name: ๐Ÿ” Did the CI definition change? - id: gate - env: - BEFORE: ${{ github.event.before }} - AFTER: ${{ github.sha }} - run: | - # A manual dispatch always runs: it exists to force a sweep. - if [ "${GITHUB_EVENT_NAME}" != "push" ]; then - echo "changed=true" >> "$GITHUB_OUTPUT" - exit 0 - fi - # A missing or unreachable `before` (a new branch, a force push, a squashed history) - # leaves the range unknowable. Sweep rather than skip: a needless recheck costs a CI - # run, a missed one leaves a gate unapplied. - if [ -z "${BEFORE}" ] || ! git cat-file -e "${BEFORE}^{commit}" 2> /dev/null; then - echo "No usable before-commit for this push โ€” sweeping." - echo "changed=true" >> "$GITHUB_OUTPUT" - exit 0 - fi - if git diff --name-only "${BEFORE}" "${AFTER}" | grep -qx '.github/workflows/ci.yaml'; then - echo "changed=true" >> "$GITHUB_OUTPUT" - else - echo "The CI definition did not change in this push โ€” nothing to recheck." - echo "changed=false" >> "$GITHUB_OUTPUT" - fi - - name: ๐Ÿ”‘ Generate GitHub App token id: app-token - if: steps.gate.outputs.changed == 'true' - # Events produced with GITHUB_TOKEN do not start new workflow runs, so a reopen - # performed with it would be silent โ€” the same reason update-agent-skills.yaml mints an - # App token to open its PR. + # Events produced with GITHUB_TOKEN do not start new workflow runs, so a reopen performed + # with it would be silent โ€” the same reason update-agent-skills.yaml mints an App token to + # open its PR. + # + # `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. 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 - name: ๐Ÿ” Re-trigger open pull requests - if: steps.gate.outputs.changed == 'true' env: GH_TOKEN: ${{ steps.app-token.outputs.token }} DRY_RUN: ${{ inputs.dry-run && '--dry-run' || '' }} diff --git a/scripts/recheck-open-prs.sh b/scripts/recheck-open-prs.sh index 1e9eddf..633c5ab 100755 --- a/scripts/recheck-open-prs.sh +++ b/scripts/recheck-open-prs.sh @@ -85,6 +85,12 @@ 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 @@ -98,7 +104,7 @@ mkdir -p "$state/closed" "$state/rearm" || exit 2 # 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 headline body + local f n st method headline body for f in "$state/closed"/*; do [ -e "$f" ] || continue n=${f##*/} @@ -118,22 +124,33 @@ settle() { 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="" echo "recheck-open-prs: restoring auto-merge on #$n" >&2 - rearm "$n" "$headline" "$body" > /dev/null 2>&1 \ + 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" } -# Re-arm auto-merge, preserving the commit metadata the request carried. Recreating it with -# defaults would silently discard a squash subject or body someone chose deliberately. +# 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 headline=$2 body=$3 - set -- "$n" --repo "$repo" --auto --squash - [ -z "$headline" ] || set -- "$@" --subject "$headline" - [ -z "$body" ] || set -- "$@" --body "$body" + 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 "$@" } @@ -190,23 +207,35 @@ while IFS=$'\t' read -r number title; do # 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. - if ! automerge=$(gh pr view "$number" --repo "$repo" --json autoMergeRequest \ - --jq 'if .autoMergeRequest == null then "none" else "armed" end'); then - echo "::error::#$number auto-merge state could not be read; left untouched" + # 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) \ + || [ -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 commit metadata before the close clears the request, so the restore can put - # back what was there instead of a default message. + # 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" - gh pr view "$number" --repo "$repo" --json autoMergeRequest \ - --jq '.autoMergeRequest.commitHeadline // ""' > "$state/rearm/$number/headline" 2> /dev/null \ - || : > "$state/rearm/$number/headline" - gh pr view "$number" --repo "$repo" --json autoMergeRequest \ - --jq '.autoMergeRequest.commitBody // ""' > "$state/rearm/$number/body" 2> /dev/null \ - || : > "$state/rearm/$number/body" + 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" fi # Close and reopen produce the `reopened` event that resolves a fresh merge ref. The head is @@ -228,9 +257,10 @@ while IFS=$'\t' read -r number title; do 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="" - if ! rearm "$number" "$headline" "$body" > /dev/null; then + 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. diff --git a/scripts/recheck-open-prs.test.sh b/scripts/recheck-open-prs.test.sh index fcfae25..23089bf 100755 --- a/scripts/recheck-open-prs.test.sh +++ b/scripts/recheck-open-prs.test.sh @@ -60,23 +60,36 @@ if [ "\$verb" = "api --paginate" ]; then fi n=\$3 +fields="" +prev="" +for a in "\$@"; do + [ "\$prev" = "--json" ] && fields=\$a + prev=\$a +done + case "\$verb" in "pr view") - field="" - for a in "\$@"; do [ "\$prev" = "--json" ] 2>/dev/null && field=\$a; prev=\$a; done - printf '%s\n' "pr view \$n \$field" >> "\$log" + printf '%s\n' "pr view \$n \$fields" >> "\$log" [ -f "\$db/viewfail" ] && exit 1 - case "\$field" in + 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") + printf '{"state":"%s","autoMergeRequest":%s}\n' "\$st" "\$am" + ;; state) - if [ -f "\$db/closed-\$n" ]; then printf 'CLOSED\n'; else printf 'OPEN\n'; fi + printf '%s\n' "\$st" ;; autoMergeRequest) - # Which projection is asked for is inferred from the --jq expression. - case "\$*" in - *commitHeadline*) cat "\$db/headline-\$n" 2>/dev/null; printf '\n' ;; - *commitBody*) cat "\$db/body-\$n" 2>/dev/null; printf '\n' ;; - *) if [ -f "\$db/am-\$n" ]; then printf 'armed\n'; else printf 'none\n'; fi ;; - esac + 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. @@ -239,11 +252,11 @@ if [ "$(grep -n 'pr close 11' "$d/calls.log" | cut -d: -f1)" -lt \ else bad "close precedes reopen for the same PR" "$log" fi -if [ "$(grep -c 'pr view 11 autoMergeRequest' "$d/calls.log")" -ge 1 ] \ - && [ "$(grep -c 'pr view 22 autoMergeRequest' "$d/calls.log")" -ge 1 ]; then - ok "auto-merge is read per PR, immediately before closing it" +if [ "$(grep -c 'pr view 11 state,autoMergeRequest' "$d/calls.log")" -ge 1 ] \ + && [ "$(grep -c 'pr view 22 state,autoMergeRequest' "$d/calls.log")" -ge 1 ]; then + ok "state and auto-merge are read per PR, immediately before closing it" else - bad "auto-merge is read per PR, immediately before closing it" "$log" + 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 @@ -358,13 +371,58 @@ make_gh "$d" "$TWO_PRS" out=$(run_script "$d") rc=$? if [ "$rc" -eq 1 ] && [ "$(grep -c 'pr close' "$d/calls.log")" -eq 0 ] \ - && [[ $out == *"auto-merge state could not be read"* ]]; then - ok "a PR whose auto-merge state cannot be read is left untouched" + && [[ $out == *"state could not be read"* ]]; then + ok "a PR whose state cannot be read is left untouched" else - bad "a PR whose auto-merge state cannot be read is left untouched" \ + 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 + # --- a malformed listing is an error, never a shorter list --------------- d="$WORK/badlisting" make_gh "$d" $'11\tfirst\nnot-a-number\tsecond\n' From c37b6ef50f93b85e5426cff81d2072b10be79f8c Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sun, 6 Sep 2026 12:59:39 +0200 Subject: [PATCH 06/10] fix(ci): wait for the reopen's own check run before restoring auto-merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings, all real, and the first is the one that mattered most: - `--auto` merges as soon as the requirements read as met, and immediately after a reopen the newest result at that commit is still the PRE-GATE green. Arming there could merge the pull request in the window before Actions creates the run for the reopen โ€” past the very gate the sweep is applying, which is the failure this whole workflow exists to prevent. The script now records the highest check-run id at the head before closing and waits for a larger one before arming, on both the direct and the recovery path. Ids only increase, so this needs no clock. When no fresh run appears it declines to arm and says so: an auto-merge a human restores is recoverable, a merge that skipped a gate is not. - A dry run shared the sweep's concurrency group, so it could displace a pending sweep while only listing โ€” leaving those PRs with no fresh event at all. Dry runs get their own group. - A dry run also minted a write-scoped App token although it performs only a listing. Its token is read-scoped now. - AGENTS.md still described the discarded `ci.yaml`-only trigger. It now documents the unconditional sweep, why the three narrower designs were rejected, and the auto-merge wait. Twenty-six cases; the new one freezes the stub's check-run high-water mark so no fresh run appears, and asserts both that auto-merge is not armed and that the PR is still left open. Removing the wait fails that case alone. Co-Authored-By: Claude Opus 5 --- .github/workflows/recheck-open-prs.yaml | 14 ++++-- AGENTS.md | 28 +++++++++--- scripts/recheck-open-prs.sh | 58 ++++++++++++++++++++++++- scripts/recheck-open-prs.test.sh | 55 ++++++++++++++++++++--- 4 files changed, 137 insertions(+), 18 deletions(-) diff --git a/.github/workflows/recheck-open-prs.yaml b/.github/workflows/recheck-open-prs.yaml index 7bb4b86..5e7b103 100644 --- a/.github/workflows/recheck-open-prs.yaml +++ b/.github/workflows/recheck-open-prs.yaml @@ -43,7 +43,12 @@ concurrency: # 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. - group: recheck-open-prs + # + # 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: {} @@ -70,12 +75,15 @@ jobs: # 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. + # + # A dry run only lists, so it takes read scopes. An advertised read-only mode should not + # be holding repository-wide write credentials it cannot use. 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-contents: ${{ inputs.dry-run && 'read' || 'write' }} + permission-pull-requests: ${{ inputs.dry-run && 'read' || 'write' }} - name: ๐Ÿ” Re-trigger open pull requests env: diff --git a/AGENTS.md b/AGENTS.md index c9b41af..0b5e9bb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -250,11 +250,19 @@ when a new job joins `CI - Required Checks` keeps the green it earned *before* t 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: any push to -`main` touching `ci.yaml` re-triggers every open PR's checks, and it can be dispatched by hand -(with a `dry-run` input) after any other change that ought to be re-evaluated. 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. +[`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 @@ -262,8 +270,14 @@ gate landed. Only a fresh `pull_request` event resolves the merge ref again, and 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 -without one ever being armed that was not. +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 check 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 diff --git a/scripts/recheck-open-prs.sh b/scripts/recheck-open-prs.sh index 633c5ab..4783a9f 100755 --- a/scripts/recheck-open-prs.sh +++ b/scripts/recheck-open-prs.sh @@ -95,6 +95,11 @@ command -v jq > /dev/null 2>&1 || { state=$(mktemp -d) || exit 2 mkdir -p "$state/closed" "$state/rearm" || exit 2 +# How long to wait for the reopened event's check run before declining to re-arm auto-merge. +# 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 @@ -104,7 +109,7 @@ mkdir -p "$state/closed" "$state/rearm" || exit 2 # 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 + local f n st method headline body sha baseline for f in "$state/closed"/*; do [ -e "$f" ] || continue n=${f##*/} @@ -127,6 +132,14 @@ settle() { 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 check 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 @@ -134,6 +147,31 @@ settle() { rm -rf "$state" } +# The highest check-run id at a commit, or 0. Ids increase, so a larger one later means a NEW +# run exists โ€” which needs no clock and no assumption about either side's timekeeping. +newest_check() { + gh api "repos/${repo}/commits/$1/check-runs" --jq '[.check_runs[].id] | max // 0' 2> /dev/null +} + +# Block until a check 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 + [ -n "$sha" ] || return 1 + while [ "$waited" -lt "$CHECK_WAIT_SECONDS" ]; do + now=$(newest_check "$sha") + case "$now" in '' | *[!0-9]*) now=0 ;; esac + [ "$now" -gt "$baseline" ] && return 0 + 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. @@ -212,7 +250,7 @@ while IFS=$'\t' read -r number title; do # 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) \ + 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)) @@ -236,6 +274,13 @@ while IFS=$'\t' read -r number title; do 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 // ""') + printf '%s' "$head_sha" > "$state/rearm/$number/sha" + # The high-water mark of check runs at this commit BEFORE the reopen, so "a run from the + # reopen exists" is answerable afterwards without trusting any clock. + check_baseline=$(newest_check "$head_sha") + case "$check_baseline" in '' | *[!0-9]*) check_baseline=0 ;; esac + 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 @@ -260,6 +305,15 @@ while IFS=$'\t' read -r number title; do 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 check 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 check 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 diff --git a/scripts/recheck-open-prs.test.sh b/scripts/recheck-open-prs.test.sh index 23089bf..0d62ba1 100755 --- a/scripts/recheck-open-prs.test.sh +++ b/scripts/recheck-open-prs.test.sh @@ -52,6 +52,14 @@ verb="\$1 \$2" # The paginated listing. Asserted on shape as well as content: the query must be passed as GET # fields, never spliced into the path, or a branch name containing & or # would select something # else entirely. +if [ "\$1" = "api" ] && case "\$2" in *check-runs) true;; *) false;; esac; then + printf '%s\n' "api check-runs" >> "\$log" + # Ids grow after a reopen, exactly as Actions creates a new run for the new event. + bumps=\$(cat "\$db/checkbump" 2>/dev/null || printf '0') + printf '%s\n' "\$bumps" + exit 0 +fi + if [ "\$verb" = "api --paginate" ]; then printf '%s\n' "api \$*" >> "\$log" [ -f "$dir/listing-fails" ] && exit 1 @@ -82,8 +90,8 @@ case "\$verb" in am=null fi case "\$fields" in - "state,autoMergeRequest") - printf '{"state":"%s","autoMergeRequest":%s}\n' "\$st" "\$am" + "state,autoMergeRequest,headRefOid") + printf '{"state":"%s","autoMergeRequest":%s,"headRefOid":"deadbeef"}\n' "\$st" "\$am" ;; state) printf '%s\n' "\$st" @@ -116,6 +124,8 @@ case "\$verb" in : > "\$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") @@ -137,14 +147,16 @@ EOF run_script() { local dir="$1" shift - env PATH="$dir/bin:$PATH" "$SCRIPT" --repo owner/name "$@" 2>&1 + 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" "$SCRIPT" "$@" 2>&1 + 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"; } @@ -252,8 +264,8 @@ if [ "$(grep -n 'pr close 11' "$d/calls.log" | cut -d: -f1)" -lt \ else bad "close precedes reopen for the same PR" "$log" fi -if [ "$(grep -c 'pr view 11 state,autoMergeRequest' "$d/calls.log")" -ge 1 ] \ - && [ "$(grep -c 'pr view 22 state,autoMergeRequest' "$d/calls.log")" -ge 1 ]; then +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" @@ -423,6 +435,37 @@ 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 check 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 check run from the reopen appears" +else + bad "auto-merge is not armed when no check 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 malformed listing is an error, never a shorter list --------------- d="$WORK/badlisting" make_gh "$d" $'11\tfirst\nnot-a-number\tsecond\n' From 9596d20071bb0009d97b15ebb0b7714889c353dd Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sun, 6 Sep 2026 13:11:17 +0200 Subject: [PATCH 07/10] fix(scripts): wait on the required check by name, and fail closed on an unreadable baseline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both refining the wait added in the previous commit: - The wait accepted ANY new check run at the head, so a run created by another integration or a manual rerun between the baseline and the reopen would satisfy it while the required workflow's own run still did not exist โ€” re-arming against the pre-gate green, which is precisely what the wait exists to prevent. Both the baseline and the poll now count only runs named by `--required-check`, defaulting to the aggregated gate this repository requires. - An unreadable baseline was converted to 0, and a zero baseline is satisfied by any historical run, so a transient failure made the wait pass instantly. The PR is now left untouched when the baseline cannot be read, and a failed poll counts as "not yet" rather than as satisfied. The check name is interpolated into a jq program, so it is validated to contain no quote or backslash. Twenty-eight cases. Assuming a zero baseline fails the new baseline case alone; dropping the name filter fails the new one plus every other armed-PR case, because with the stub the unfiltered query reads a counter the reopen never moves โ€” an artifact of the fixture, not a partition, and stated rather than dressed up as one. Co-Authored-By: Claude Opus 5 --- scripts/recheck-open-prs.sh | 62 ++++++++++++++++++++++++++------ scripts/recheck-open-prs.test.sh | 54 +++++++++++++++++++++++++--- 2 files changed, 101 insertions(+), 15 deletions(-) diff --git a/scripts/recheck-open-prs.sh b/scripts/recheck-open-prs.sh index 4783a9f..a2cc8f0 100755 --- a/scripts/recheck-open-prs.sh +++ b/scripts/recheck-open-prs.sh @@ -43,7 +43,7 @@ set -uo pipefail usage() { cat >&2 <<'EOF' -usage: recheck-open-prs.sh --repo OWNER/NAME [--base BRANCH] [--dry-run] +usage: recheck-open-prs.sh --repo OWNER/NAME [--base BRANCH] [--required-check NAME] [--dry-run] EOF exit 2 } @@ -51,6 +51,10 @@ EOF repo="" base="main" dry_run=0 +# The check whose fresh run proves the reopen has been picked up. Named rather than "any check", +# because another integration's run would otherwise satisfy the wait while the required +# workflow's own run for the reopen still did not exist. +required_check="CI - Required Checks" while [ "$#" -gt 0 ]; do case "$1" in @@ -64,6 +68,11 @@ while [ "$#" -gt 0 ]; do base=$2 shift 2 ;; + --required-check) + [ "$#" -ge 2 ] || usage + required_check=$2 + shift 2 + ;; --dry-run) dry_run=1 shift @@ -81,6 +90,15 @@ case "$repo" in *) usage ;; esac +# The name is interpolated into a jq program, so a quote or backslash in it would change that +# program rather than the value it compares against. +if [ -z "$required_check" ] \ + || [ "${required_check//\"/}" != "$required_check" ] \ + || [ "${required_check//\\/}" != "$required_check" ]; then + echo "recheck-open-prs: --required-check must be a plain name (no quotes or backslashes)" >&2 + exit 2 +fi + command -v gh > /dev/null 2>&1 || { echo "recheck-open-prs: gh is required" >&2 exit 2 @@ -147,10 +165,18 @@ settle() { rm -rf "$state" } -# The highest check-run id at a commit, or 0. Ids increase, so a larger one later means a NEW -# run exists โ€” which needs no clock and no assumption about either side's timekeeping. +# The highest id among check runs NAMED $required_check at a commit, or 0 when it has none. Ids +# increase, so a larger one later means a new run of that check exists โ€” which needs no clock and +# no assumption about either side's timekeeping. +# +# Filtered by name on purpose. Any check run would satisfy an unfiltered comparison, including one +# another integration or a manual rerun created after the baseline, so the wait could pass while +# the required workflow's own run for the reopen still did not exist. +# +# Exits non-zero when the read fails, so a caller can tell "no runs yet" (0) from "unknown". newest_check() { - gh api "repos/${repo}/commits/$1/check-runs" --jq '[.check_runs[].id] | max // 0' 2> /dev/null + gh api "repos/${repo}/commits/$1/check-runs" \ + --jq "[.check_runs[] | select(.name == \"${required_check}\") | .id] | max // 0" 2> /dev/null } # Block until a check run newer than $2 exists at commit $1. Auto-merge means "merge once the @@ -161,11 +187,15 @@ newest_check() { # 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 - now=$(newest_check "$sha") - case "$now" in '' | *[!0-9]*) now=0 ;; esac - [ "$now" -gt "$baseline" ] && return 0 + # A failed poll is "not yet", never "satisfied" โ€” the loop simply keeps waiting. + if now=$(newest_check "$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 @@ -275,11 +305,21 @@ while IFS=$'\t' read -r number title; do 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_check "$head_sha") \ + || case "$check_baseline" in '' | *[!0-9]*) true ;; *) false ;; esac \ + || [ -z "$head_sha" ]; then + echo "::error::#$number check 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" - # The high-water mark of check runs at this commit BEFORE the reopen, so "a run from the - # reopen exists" is answerable afterwards without trusting any clock. - check_baseline=$(newest_check "$head_sha") - case "$check_baseline" in '' | *[!0-9]*) check_baseline=0 ;; esac printf '%s' "$check_baseline" > "$state/rearm/$number/baseline" fi diff --git a/scripts/recheck-open-prs.test.sh b/scripts/recheck-open-prs.test.sh index 0d62ba1..0b0ff28 100755 --- a/scripts/recheck-open-prs.test.sh +++ b/scripts/recheck-open-prs.test.sh @@ -53,10 +53,14 @@ verb="\$1 \$2" # fields, never spliced into the path, or a branch name containing & or # would select something # else entirely. if [ "\$1" = "api" ] && case "\$2" in *check-runs) true;; *) false;; esac; then - printf '%s\n' "api check-runs" >> "\$log" - # Ids grow after a reopen, exactly as Actions creates a new run for the new event. - bumps=\$(cat "\$db/checkbump" 2>/dev/null || printf '0') - printf '%s\n' "\$bumps" + printf '%s\n' "api check-runs \$*" >> "\$log" + [ -f "\$db/checkfail" ] && exit 1 + # The script filters by the required check's name, so an id only counts when the query asks + # for that name. A run from some OTHER integration must not satisfy the wait. + case "\$*" in + *"CI - Required Checks"*) printf '%s\n' "\$(cat "\$db/checkbump" 2>/dev/null || printf '0')" ;; + *) printf '%s\n' "\$(cat "\$db/othercheck" 2>/dev/null || printf '0')" ;; + esac exit 0 fi @@ -466,6 +470,48 @@ else bad "declining to re-arm still leaves the PR open" "$(left_closed "$d") still closed" fi +# --- another integration's check does not satisfy the wait --------------- +# The wait must observe a new run of the REQUIRED check. Any-check-counts would pass while the +# required workflow's own run for the reopen still did not exist, and re-arm against the +# pre-gate green. +d="$WORK/othercheck" +make_gh "$d" "$TWO_PRS" "" "11" +python3 - "$d/bin/gh" <<'PYEOF' +import sys +p=sys.argv[1] +s=open(p).read() +# The required check never gains a run; an unrelated integration's does. +s=s.replace('printf \'%s\' "$(( $(cat "$db/checkbump" 2>/dev/null || printf \'0\') + 1 ))" > "$db/checkbump"', + 'printf \'%s\' "$(( $(cat "$db/othercheck" 2>/dev/null || printf \'0\') + 1 ))" > "$db/othercheck"') +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 new check from another integration does not satisfy the wait" +else + bad "a new check from another integration does not satisfy the wait" \ + "exit $rc" "$(cat "$d/calls.log")" "$out" +fi + +# --- an unreadable check 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 == *"check baseline could not be read"* ]]; then + ok "an unreadable check baseline leaves the PR untouched, never assumed zero" +else + bad "an unreadable check 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' From 1e0d58cc4cfeea32737d59a203a2e2640cb4d16e Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sun, 6 Sep 2026 13:19:48 +0200 Subject: [PATCH 08/10] fix(scripts): tie the wait to a new pull_request run, not a new check run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A manual rerun of the existing CI run creates fresh check runs โ€” new ids, same name โ€” while no run for the reopen exists, so the name-filtered check-run comparison could still be satisfied by the pre-gate result and re-arm auto-merge against it. Workflow runs do not behave that way: a rerun keeps its run id and adds an attempt, so a new `pull_request` run id can only come from a new `pull_request` event, which is exactly what the wait is for. The baseline and the poll now read `actions/runs` filtered to `event=pull_request` at the head, with the parameters passed as GET fields rather than spliced into the path. Another such event satisfying it โ€” a push, say โ€” is correct rather than a gap: it also resolves a fresh merge ref. This also removes the `--required-check` option added one commit ago, since the workflow's name is no longer part of the test, and with it the jq interpolation that name required. Twenty-eight cases. A new one freezes the workflow-run high-water mark, modelling a rerun that creates no new run, and asserts auto-merge is not armed and the PR stays open. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 2 +- scripts/recheck-open-prs.sh | 61 +++++++++++++------------------- scripts/recheck-open-prs.test.sh | 47 ++++++++++++------------ 3 files changed, 47 insertions(+), 63 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0b5e9bb..81029b5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -272,7 +272,7 @@ runs under an App token because events produced with `GITHUB_TOKEN` start no wor [`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 check run +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 diff --git a/scripts/recheck-open-prs.sh b/scripts/recheck-open-prs.sh index a2cc8f0..0dcbc27 100755 --- a/scripts/recheck-open-prs.sh +++ b/scripts/recheck-open-prs.sh @@ -43,7 +43,7 @@ set -uo pipefail usage() { cat >&2 <<'EOF' -usage: recheck-open-prs.sh --repo OWNER/NAME [--base BRANCH] [--required-check NAME] [--dry-run] +usage: recheck-open-prs.sh --repo OWNER/NAME [--base BRANCH] [--dry-run] EOF exit 2 } @@ -51,10 +51,6 @@ EOF repo="" base="main" dry_run=0 -# The check whose fresh run proves the reopen has been picked up. Named rather than "any check", -# because another integration's run would otherwise satisfy the wait while the required -# workflow's own run for the reopen still did not exist. -required_check="CI - Required Checks" while [ "$#" -gt 0 ]; do case "$1" in @@ -68,11 +64,6 @@ while [ "$#" -gt 0 ]; do base=$2 shift 2 ;; - --required-check) - [ "$#" -ge 2 ] || usage - required_check=$2 - shift 2 - ;; --dry-run) dry_run=1 shift @@ -90,15 +81,6 @@ case "$repo" in *) usage ;; esac -# The name is interpolated into a jq program, so a quote or backslash in it would change that -# program rather than the value it compares against. -if [ -z "$required_check" ] \ - || [ "${required_check//\"/}" != "$required_check" ] \ - || [ "${required_check//\\/}" != "$required_check" ]; then - echo "recheck-open-prs: --required-check must be a plain name (no quotes or backslashes)" >&2 - exit 2 -fi - command -v gh > /dev/null 2>&1 || { echo "recheck-open-prs: gh is required" >&2 exit 2 @@ -113,7 +95,7 @@ command -v jq > /dev/null 2>&1 || { state=$(mktemp -d) || exit 2 mkdir -p "$state/closed" "$state/rearm" || exit 2 -# How long to wait for the reopened event's check run before declining to re-arm auto-merge. +# 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} @@ -155,7 +137,7 @@ settle() { # 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 check 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 + 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 @@ -165,21 +147,26 @@ settle() { rm -rf "$state" } -# The highest id among check runs NAMED $required_check at a commit, or 0 when it has none. Ids -# increase, so a larger one later means a new run of that check exists โ€” which needs no clock and -# no assumption about either side's timekeeping. +# The highest WORKFLOW RUN id for a `pull_request` event at a commit, or 0 when it has none. # -# Filtered by name on purpose. Any check run would satisfy an unfiltered comparison, including one -# another integration or a manual rerun created after the baseline, so the wait could pass while -# the required workflow's own run for the reopen still did not exist. +# 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. # -# Exits non-zero when the read fails, so a caller can tell "no runs yet" (0) from "unknown". -newest_check() { - gh api "repos/${repo}/commits/$1/check-runs" \ - --jq "[.check_runs[] | select(.name == \"${required_check}\") | .id] | max // 0" 2> /dev/null +# 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 check run newer than $2 exists at commit $1. Auto-merge means "merge once the +# 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 @@ -193,7 +180,7 @@ await_fresh_check() { 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_check "$sha") && case "$now" in '' | *[!0-9]*) false ;; *) true ;; esac; then + 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" @@ -311,10 +298,10 @@ while IFS=$'\t' read -r number title; do # 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_check "$head_sha") \ + if ! check_baseline=$(newest_pr_run "$head_sha") \ || case "$check_baseline" in '' | *[!0-9]*) true ;; *) false ;; esac \ || [ -z "$head_sha" ]; then - echo "::error::#$number check baseline could not be read; left untouched (its auto-merge could not be safely restored)" + 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 @@ -345,11 +332,11 @@ while IFS=$'\t' read -r number title; do 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 check run before arming. Until it exists the newest result at + # 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 check 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." + 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 diff --git a/scripts/recheck-open-prs.test.sh b/scripts/recheck-open-prs.test.sh index 0b0ff28..79bfc19 100755 --- a/scripts/recheck-open-prs.test.sh +++ b/scripts/recheck-open-prs.test.sh @@ -52,15 +52,12 @@ verb="\$1 \$2" # The paginated listing. Asserted on shape as well as content: the query must be passed as GET # fields, never spliced into the path, or a branch name containing & or # would select something # else entirely. -if [ "\$1" = "api" ] && case "\$2" in *check-runs) true;; *) false;; esac; then - printf '%s\n' "api check-runs \$*" >> "\$log" +if [ "\$1" = "api" ] && case "\$*" in *actions/runs*) true;; *) false;; esac; then + printf '%s\n' "api actions/runs \$*" >> "\$log" [ -f "\$db/checkfail" ] && exit 1 - # The script filters by the required check's name, so an id only counts when the query asks - # for that name. A run from some OTHER integration must not satisfy the wait. - case "\$*" in - *"CI - Required Checks"*) printf '%s\n' "\$(cat "\$db/checkbump" 2>/dev/null || printf '0')" ;; - *) printf '%s\n' "\$(cat "\$db/othercheck" 2>/dev/null || printf '0')" ;; - esac + # 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 @@ -446,7 +443,7 @@ fi # 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 check run. +# 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] @@ -459,9 +456,9 @@ 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 check run from the reopen appears" + ok "auto-merge is not armed when no run from the reopen appears" else - bad "auto-merge is not armed when no check run from the reopen appears" "exit $rc" "$log" "$out" + 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 @@ -470,32 +467,32 @@ else bad "declining to re-arm still leaves the PR open" "$(left_closed "$d") still closed" fi -# --- another integration's check does not satisfy the wait --------------- -# The wait must observe a new run of the REQUIRED check. Any-check-counts would pass while the -# required workflow's own run for the reopen still did not exist, and re-arm against the -# pre-gate green. -d="$WORK/othercheck" +# --- 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() -# The required check never gains a run; an unrelated integration's does. -s=s.replace('printf \'%s\' "$(( $(cat "$db/checkbump" 2>/dev/null || printf \'0\') + 1 ))" > "$db/checkbump"', - 'printf \'%s\' "$(( $(cat "$db/othercheck" 2>/dev/null || printf \'0\') + 1 ))" > "$db/othercheck"') +# 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 new check from another integration does not satisfy the wait" + ok "a rerun that creates no new pull_request run does not satisfy the wait" else - bad "a new check from another integration does not satisfy the wait" \ + 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 check baseline leaves the PR untouched ---------------- +# --- 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" @@ -505,10 +502,10 @@ 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 == *"check baseline could not be read"* ]]; then - ok "an unreadable check baseline leaves the PR untouched, never assumed zero" + && [[ $out == *"run baseline could not be read"* ]]; then + ok "an unreadable run baseline leaves the PR untouched, never assumed zero" else - bad "an unreadable check baseline leaves the PR untouched, never assumed zero" \ + bad "an unreadable run baseline leaves the PR untouched, never assumed zero" \ "exit $rc" "$(cat "$d/calls.log")" "$out" fi From 070767204039330667ca0274f68ca83f37e2636c Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sun, 6 Sep 2026 13:26:17 +0200 Subject: [PATCH 09/10] fix(ci): grant the App token Actions read for the workflow-run wait MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit moved the pre-arm wait from check runs to `pull_request` workflow runs, and that endpoint needs an Actions read scope the token did not request. The baseline read would have 403'd on every pull request with auto-merge armed, leaving each one untouched with its stale green and its auto-merge still armed โ€” bypassing the gate on exactly the pull requests that were about to merge themselves. Read-only, and requested in both modes: the dry run does not reach that endpoint, and a scope it cannot use is better than a mode-dependent one that is wrong in the mode that matters. Co-Authored-By: Claude Opus 5 --- .github/workflows/recheck-open-prs.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/recheck-open-prs.yaml b/.github/workflows/recheck-open-prs.yaml index 5e7b103..711c566 100644 --- a/.github/workflows/recheck-open-prs.yaml +++ b/.github/workflows/recheck-open-prs.yaml @@ -84,6 +84,12 @@ jobs: private-key: ${{ secrets.APP_PRIVATE_KEY }} permission-contents: ${{ inputs.dry-run && 'read' || 'write' }} permission-pull-requests: ${{ inputs.dry-run && 'read' || 'write' }} + # Reading workflow runs needs its own scope. 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 โ€” so without this 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. + permission-actions: read - name: ๐Ÿ” Re-trigger open pull requests env: From ff14aa8c6fc423f01397e186b18e2b37b6fe72f5 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sun, 6 Sep 2026 13:34:41 +0200 Subject: [PATCH 10/10] fix(ci): give the dry run a token with only the scopes it uses The Actions read scope added for the workflow-run wait was granted in both modes, but a dry run never reaches that endpoint, so an advertised read-only mode was holding a permission it cannot use. Split into two token steps rather than more conditional scopes: the modes need different SETS of permissions, not different levels of the same ones, and the action takes a fixed list, so a scope can be lowered by an expression but not dropped by one. The sweep keeps contents write, pull-requests write and actions read, each justified in place; the dry run gets contents read and pull-requests read. The previous commit argued an unconditional grant was safer than a mode-dependent one. That was wrong: the risk it named was an expression being wrong in the mutating mode, which two separate steps remove entirely. Co-Authored-By: Claude Opus 5 --- .github/workflows/recheck-open-prs.yaml | 45 ++++++++++++++++--------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/.github/workflows/recheck-open-prs.yaml b/.github/workflows/recheck-open-prs.yaml index 711c566..cbe08d4 100644 --- a/.github/workflows/recheck-open-prs.yaml +++ b/.github/workflows/recheck-open-prs.yaml @@ -65,35 +65,50 @@ jobs: with: persist-credentials: false - - name: ๐Ÿ”‘ Generate GitHub App token + # 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 - # Events produced with GITHUB_TOKEN do not start new workflow runs, so a reopen performed - # with it would be silent โ€” the same reason update-agent-skills.yaml mints an App token to - # open its PR. - # + 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. # - # A dry run only lists, so it takes read scopes. An advertised read-only mode should not - # be holding repository-wide write credentials it cannot use. + # `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: ${{ inputs.dry-run && 'read' || 'write' }} - permission-pull-requests: ${{ inputs.dry-run && 'read' || 'write' }} - # Reading workflow runs needs its own scope. 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 โ€” so without this 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. + 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 }} + 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