From 60028332755a120acdd28d4afccd2ac8e6afe8ad Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 20:46:12 +0700 Subject: [PATCH 01/14] ci: gate the two things that were outside every gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two holes, both found by auditing merges rather than code, both now checked by something that fails on the real historical defect. 1. scripts/merge-preflight.sh — refuses a merge that violates either half of "the final SHA was approved AND every check is green". Both halves were violated this week in opposite ways and neither was visible in the fields I was reading: #400 #402 #404 #405 merged with a FAILING check. mergeStateStatus said UNSTABLE, which MEANS "a check failed but merging is not blocked" -- I read it as "mergeable". #408 merged a commit nobody reviewed. The approval named b327086b, a background push moved the branch to 4b5084e7, and `gh pr merge` takes the BRANCH. Run against the real history: #400 FAIL (both legs), #405 FAIL (checks), #408 FAIL (SHA), #398 PASS. The control is #398 -- a gate that only ever says no is not distinguishable from a broken one. It also refuses an EMPTY check list rather than reading silence as green. 2. scripts/check-abi-bundle.mjs — compares abis/*.json against out/ by full shape: name, inputs, outputs, stateMutability. gen:abi-docs:check recomputes docs/abi/*.md and never reads abis/, which is the bundle sync_to_sdk.sh copies into aastar-sdk, so the artifact with consumers was outside the gate while the docs were inside it (#411, raised by pr-daemon). Names are not enough: #400 added a field to an EXISTING getter (guardianSlashCases 7 -> 8 outputs) without adding or removing a function. Three sides: drop one output from guardianSlashCases -> new check exit 1, naming the shape the same mutation -> gen:abi-docs:check exit 0, blind restored -> exit 0 The middle row is why this exists. Scoped to contracts under contracts/src/. Its first version compared everything and flagged EntryPoint, SimpleAccount and SimpleAccountFactory -- account-abstraction v0.7 ABIs deliberately pinned to what is DEPLOYED. A gate that cries wolf on pinned externals gets ignored, and then it is not a gate. What it does not compare is printed, not silently counted as agreement. abis/** added to both paths filters -- a PR touching only that directory did not even trigger the job. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- .github/workflows/abi-docs.yml | 13 ++++++ scripts/check-abi-bundle.mjs | 82 ++++++++++++++++++++++++++++++++++ scripts/merge-preflight.sh | 61 +++++++++++++++++++++++++ 3 files changed, 156 insertions(+) create mode 100755 scripts/check-abi-bundle.mjs create mode 100755 scripts/merge-preflight.sh diff --git a/.github/workflows/abi-docs.yml b/.github/workflows/abi-docs.yml index 043e1685..2bc0d597 100644 --- a/.github/workflows/abi-docs.yml +++ b/.github/workflows/abi-docs.yml @@ -10,7 +10,11 @@ on: paths: - 'contracts/src/**' - 'scripts/gen-abi-docs.mjs' + - 'abis/**' + - 'scripts/check-abi-bundle.mjs' - 'docs/abi/**' + - 'abis/**' + - 'scripts/check-abi-bundle.mjs' - 'package.json' - 'pnpm-lock.yaml' - '.github/workflows/abi-docs.yml' @@ -19,6 +23,8 @@ on: - 'contracts/src/**' - 'scripts/gen-abi-docs.mjs' - 'docs/abi/**' + - 'abis/**' + - 'scripts/check-abi-bundle.mjs' - 'package.json' - 'pnpm-lock.yaml' - '.github/workflows/abi-docs.yml' @@ -52,3 +58,10 @@ jobs: - name: Verify ABI docs are up to date (pnpm gen:abi-docs:check) run: pnpm gen:abi-docs:check + + # gen:abi-docs:check only recomputes docs/abi/*.md. It never reads + # abis/*.json -- the bundle sync_to_sdk.sh copies into aastar-sdk. That is + # how abis/BLSAggregator.json drifted four functions behind the contract + # for six days with this job green (#410, #411). + - name: ABI bundles match the compiled contracts + run: node scripts/check-abi-bundle.mjs diff --git a/scripts/check-abi-bundle.mjs b/scripts/check-abi-bundle.mjs new file mode 100755 index 00000000..0fdc9d50 --- /dev/null +++ b/scripts/check-abi-bundle.mjs @@ -0,0 +1,82 @@ +#!/usr/bin/env node +// ============================================================================= +// check-abi-bundle.mjs +// +// `gen-abi-docs.mjs --check` recomputes docs/abi/*.md and nothing else. It never +// reads abis/*.json — the bundle sync_to_sdk.sh copies into aastar-sdk, i.e. the +// artifact downstream actually imports. So the file with consumers sat outside +// the gate while the human-readable docs sat inside it, and abis/BLSAggregator.json +// drifted four functions behind the contract for six days with CI green. Raised +// by pr-daemon; issue #411. +// +// Names are not enough. #400 added a field to an EXISTING getter +// (guardianSlashCases, 7 outputs -> 8) without adding or removing a function, so a +// name-level diff sees nothing. This compares the full shape: inputs, outputs, +// stateMutability. +// ============================================================================= +import { readFileSync, existsSync, readdirSync } from "node:fs"; +import { join } from "node:path"; + +const ROOT = process.cwd(); +const ABIS = join(ROOT, "abis"); +const OUT = join(ROOT, "out"); + +const shape = (f) => JSON.stringify({ + t: f.type, n: f.name ?? "", + i: (f.inputs ?? []).map((x) => x.type), + o: (f.outputs ?? []).map((x) => x.type), + m: f.stateMutability ?? "", +}); + +// Only first-party contracts are comparable. abis/ also carries EntryPoint, +// SimpleAccount and SimpleAccountFactory, which are account-abstraction v0.7 +// artifacts deliberately pinned to what is DEPLOYED (EntryPoint v0.7 lives at +// 0x0000000071727De22E5E9d8BAf0edAc6f37da032). Comparing those against whatever +// `out/` happens to hold reports drift that is intentional — the first version +// of this script did exactly that and flagged all three. A gate that cries wolf +// on pinned externals gets ignored, and then it is not a gate. +const FIRST_PARTY = new Set( + walkSol(join(ROOT, "contracts", "src")).map((f) => f.replace(/\.sol$/, "")) +); +function walkSol(dir) { + if (!existsSync(dir)) return []; + return readdirSync(dir, { withFileTypes: true }).flatMap((e) => + e.isDirectory() ? walkSol(join(dir, e.name)) : e.name.endsWith(".sol") ? [e.name] : [] + ); +} + +function compiledAbi(name) { + const p = join(OUT, `${name}.sol`, `${name}.json`); + return existsSync(p) ? JSON.parse(readFileSync(p, "utf8")).abi : null; +} + +let stale = 0, checked = 0, skipped = [], external = []; +for (const file of readdirSync(ABIS).filter((f) => f.endsWith(".json"))) { + const name = file.replace(/\.json$/, ""); + if (name === "abi.config") continue; + const raw = JSON.parse(readFileSync(join(ABIS, file), "utf8")); + const committed = Array.isArray(raw) ? raw : raw.abi; + const compiled = compiledAbi(name); + // No artifact means this bundle has no contract to compare against. Say so + // rather than counting it as agreement: an empty comparison and a passing one + // are the same reading otherwise. + if (!FIRST_PARTY.has(name)) { external.push(name); continue; } + if (!compiled || !committed) { skipped.push(name); continue; } + checked++; + const c = new Set(committed.map(shape)); + const o = new Set(compiled.map(shape)); + const missing = [...o].filter((x) => !c.has(x)); + const extra = [...c].filter((x) => !o.has(x)); + if (missing.length || extra.length) { + stale++; + console.error(`STALE: abis/${file}`); + for (const m of missing.slice(0, 6)) console.error(` only in compiled: ${m}`); + for (const e of extra.slice(0, 6)) console.error(` only in abis/ : ${e}`); + } +} +console.log(`compared ${checked} bundles against out/`); +if (skipped.length) console.log(`no artifact, NOT compared: ${skipped.join(", ")}`); +if (external.length) console.log(`external / pinned to a deployment, NOT compared: ${external.join(", ")}`); +if (checked === 0) { console.error("FAIL: nothing was compared — run `forge build` first"); process.exit(2); } +if (stale) { console.error(`\n${stale} bundle(s) stale — run scripts/extract_v3_abis.sh and commit.`); process.exit(1); } +console.log("abis/ matches the compiled contracts (full shape, not just names)"); diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh new file mode 100755 index 00000000..2e852b5b --- /dev/null +++ b/scripts/merge-preflight.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# ============================================================================= +# merge-preflight.sh +# +# Refuses a merge that would violate either half of "the final SHA was approved +# AND every check is green". Both halves were violated on this repo in the same +# week, in opposite ways, and neither was visible in the fields I was reading: +# +# #400 #402 #404 #405 merged with a FAILING check. `mergeStateStatus` said +# UNSTABLE, which means "a check failed but merging is +# not blocked" — I read it as "mergeable". +# #408 merged a commit nobody reviewed. The approval named +# b327086b, a background push moved the branch to +# 4b5084e7, and `gh pr merge` takes the BRANCH, not the +# SHA. I had read the head several steps earlier. +# +# Neither `reviewDecision` nor `mergeStateStatus` answers either question, which +# is why checking them felt like checking. +# +# Exit 0 = safe to merge. Any other exit = do not merge. +# ============================================================================= +set -uo pipefail +PR="${1:?usage: merge-preflight.sh }" +REPO="${REPO:-AAStarCommunity/SuperPaymaster}" +fail=0 + +head=$(gh pr view "$PR" --repo "$REPO" --json headRefOid -q .headRefOid 2>/dev/null | tr -d ' \n') +[ -n "$head" ] || { echo "FAIL could not read the PR head — refusing on an unread value"; exit 3; } +echo "head at this moment : $head" + +# --- 1. the approval must name THIS commit ------------------------------------ +appr=$(gh api "repos/$REPO/pulls/$PR/reviews" \ + --jq '[.[]|select(.state=="APPROVED")|.commit_id]|last' 2>/dev/null | tr -d '" \n') +if [ -z "$appr" ] || [ "$appr" = "null" ]; then + echo "FAIL no APPROVED review found"; fail=1 +elif [ "$appr" != "$head" ]; then + echo "FAIL the approval names $appr, the branch is at $head" + echo " An approval is a statement about a SHA. Merging takes a branch." + fail=1 +else + echo "OK approved SHA == head" +fi + +# --- 2. no check may be failing ----------------------------------------------- +runs=$(gh api "repos/$REPO/commits/$head/check-runs" --jq '.check_runs|length' 2>/dev/null | tr -d ' \n') +if [ -z "$runs" ] || [ "$runs" = "0" ]; then + # An empty check list is not a green one. Refuse rather than read silence as success. + echo "FAIL no check runs reported for $head — cannot distinguish 'all green' from 'never ran'" + fail=1 +else + bad=$(gh api "repos/$REPO/commits/$head/check-runs" \ + --jq '[.check_runs[]|select(.conclusion=="failure" or .conclusion=="timed_out" or .conclusion=="cancelled")|.name]|join(", ")' 2>/dev/null | tr -d '"') + pend=$(gh api "repos/$REPO/commits/$head/check-runs" \ + --jq '[.check_runs[]|select(.status!="completed")|.name]|join(", ")' 2>/dev/null | tr -d '"') + if [ -n "$bad" ]; then echo "FAIL failing checks: $bad"; fail=1; fi + if [ -n "$pend" ]; then echo "FAIL still running: $pend"; fail=1; fi + [ -z "$bad" ] && [ -z "$pend" ] && echo "OK $runs check runs, none failing or pending" +fi + +[ "$fail" -eq 0 ] && echo "PREFLIGHT PASS — safe to merge $PR at $head" || echo "PREFLIGHT FAIL — do not merge $PR" +exit "$fail" From cb0af21d92ceeab6ff7a555373a41125d2e6c8fa Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 20:54:20 +0700 Subject: [PATCH 02/14] fix(ci): close the fail-open paths in both new gates, and make the preflight run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex and pr-daemon each found a way the gates added in this PR could wrongly pass. Both are the defect the gates exist to catch, reproduced inside the gates. 1. merge-preflight.sh read `gh api` output with `x=$(api ...)`, and `api` tried to `exit 4` on a failed lookup. Command substitution runs in a SUBSHELL, so that exit ended the subshell and the caller continued with an empty string — a broken lookup printed FAIL to stderr and the run went on to PASS on empty values. Found by breaking one lookup and noticing the exit code came back 1 for an unrelated reason (an empty `total`) rather than refusing on the lookup itself: the right answer for the wrong cause. `api` now assigns into the caller's variable with `printf -v` and returns non-zero; every call site refuses on it. Probed on the path that previously passed — break the `bad` lookup against the COMPLIANT PR and it exits 4 instead of PASS. Also now covers what it did not: the Status API (`/status`) is separate from check-runs, so a red commit status was invisible; a CHANGES_REQUESTED newer than the newest approval blocks; both listings paginate. 2. check-abi-bundle.mjs pushed a first-party bundle with no compiled artifact to `skipped` and exited 0, so a partial or stale `out/` turned every uncompared bundle into silent agreement. A missing artifact is now a failure. 3. Nothing invoked merge-preflight.sh. It was a command to remember, and the failure it guards is forgetting — #408 was merged by someone who knew the rule, minutes after reading a `gh pr view` that had gone stale. Now a pull_request job. Make it a required check to turn advice into a refusal. Regression unchanged on real history: #400 FAIL (both legs), #405 FAIL (checks), #408 FAIL (sha), #398 PASS. The control is #398 — a gate that only says no is not distinguishable from a broken one. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- .github/workflows/merge-preflight.yml | 41 ++++++++++ scripts/check-abi-bundle.mjs | 7 +- scripts/merge-preflight.sh | 109 +++++++++++++++++++------- 3 files changed, 128 insertions(+), 29 deletions(-) create mode 100644 .github/workflows/merge-preflight.yml diff --git a/.github/workflows/merge-preflight.yml b/.github/workflows/merge-preflight.yml new file mode 100644 index 00000000..03d0f162 --- /dev/null +++ b/.github/workflows/merge-preflight.yml @@ -0,0 +1,41 @@ +# ============================================================================= +# The preflight has to RUN, not merely exist. +# +# scripts/merge-preflight.sh was added as a command someone would remember to +# type — and the failure it guards is forgetting. #408 was merged by someone who +# knew the rule, minutes after reading a `gh pr view` that had already gone +# stale. A rule that is "sometimes done" is done "often forgotten". Raised by +# pr-daemon on #412. +# +# Both legs are observable at push time: the approval-vs-head comparison is why +# it re-reads the head on every push rather than trusting an earlier reading. +# Make this a REQUIRED check to turn it from advice into a refusal. +# ============================================================================= +name: merge-preflight + +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + pull_request_review: + types: [submitted, dismissed] + +permissions: + contents: read + pull-requests: read + checks: read + statuses: read + +jobs: + preflight: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + # Its own checks are still running while it runs, so a red "still running" + # here is expected mid-CI and clears on the last push once everything + # settles. What it catches immediately and permanently is the approval + # naming a different SHA than the branch — the #408 shape. + - name: Approval names this SHA, and nothing is failing + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + run: ./scripts/merge-preflight.sh ${{ github.event.pull_request.number }} diff --git a/scripts/check-abi-bundle.mjs b/scripts/check-abi-bundle.mjs index 0fdc9d50..246d3f26 100755 --- a/scripts/check-abi-bundle.mjs +++ b/scripts/check-abi-bundle.mjs @@ -61,7 +61,12 @@ for (const file of readdirSync(ABIS).filter((f) => f.endsWith(".json"))) { // rather than counting it as agreement: an empty comparison and a passing one // are the same reading otherwise. if (!FIRST_PARTY.has(name)) { external.push(name); continue; } - if (!compiled || !committed) { skipped.push(name); continue; } + // A first-party bundle whose artifact is missing is NOT a pass. The first + // version pushed it to `skipped` and exited 0, so a partial or stale `out/` + // turned every uncompared bundle into a silent agreement — the same + // fail-open this script exists to close, reproduced inside it. + if (!compiled) { console.error(`FAIL: abis/${file} — no compiled artifact at out/${name}.sol/${name}.json`); stale++; continue; } + if (!committed) { console.error(`FAIL: abis/${file} — unreadable or empty ABI`); stale++; continue; } checked++; const c = new Set(committed.map(shape)); const o = new Set(compiled.map(shape)); diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh index 2e852b5b..fb68e317 100755 --- a/scripts/merge-preflight.sh +++ b/scripts/merge-preflight.sh @@ -2,36 +2,73 @@ # ============================================================================= # merge-preflight.sh # -# Refuses a merge that would violate either half of "the final SHA was approved -# AND every check is green". Both halves were violated on this repo in the same -# week, in opposite ways, and neither was visible in the fields I was reading: +# Refuses a merge that violates "the final SHA was approved AND every check is +# green". Both halves were violated on this repo in the same week: # # #400 #402 #404 #405 merged with a FAILING check. `mergeStateStatus` said -# UNSTABLE, which means "a check failed but merging is -# not blocked" — I read it as "mergeable". +# UNSTABLE, which MEANS "a check failed but merging is +# not blocked" — read as "mergeable". # #408 merged a commit nobody reviewed. The approval named # b327086b, a background push moved the branch to -# 4b5084e7, and `gh pr merge` takes the BRANCH, not the -# SHA. I had read the head several steps earlier. +# 4b5084e7, and `gh pr merge` takes the BRANCH. # -# Neither `reviewDecision` nor `mergeStateStatus` answers either question, which -# is why checking them felt like checking. +# EVERY LOOKUP FAILS CLOSED. The first version read `gh api` output into a +# variable and tested it for emptiness — so an auth error, a rate limit or a +# typo'd path produced "" and was indistinguishable from "nothing is failing". +# A gate whose instrument failure looks like success is the defect it exists to +# catch. Each call's exit status is now checked before its output is believed. # -# Exit 0 = safe to merge. Any other exit = do not merge. +# Exit 0 = safe to merge. Anything else = do not merge. # ============================================================================= set -uo pipefail PR="${1:?usage: merge-preflight.sh }" REPO="${REPO:-AAStarCommunity/SuperPaymaster}" fail=0 +# api +# +# Assigns into the CALLER's variable with printf -v and returns non-zero on +# failure. It must not be used as `x=$(api ...)`: command substitution runs in a +# subshell, so an `exit` inside it exits only the subshell and the caller sails on +# with an empty string. The first version did exactly that — a broken lookup +# printed FAIL to stderr and the run continued to PASS on empty values, which is +# the fail-open this gate exists to close, reproduced inside the gate. Caught by +# breaking one lookup and watching the exit code come back 1 for an unrelated +# reason instead of refusing on the lookup. +api() { + local __var="$1" __jq="$2"; shift 2 + local __out __rc + __out=$(gh api "$@" --jq "$__jq" 2>/dev/null); __rc=$? + if [ $__rc -ne 0 ]; then + echo "FAIL lookup failed (gh api $*) — refusing on an unread value, not passing" + fail=1 + return 1 + fi + printf -v "$__var" '%s' "$__out" + return 0 +} + head=$(gh pr view "$PR" --repo "$REPO" --json headRefOid -q .headRefOid 2>/dev/null | tr -d ' \n') -[ -n "$head" ] || { echo "FAIL could not read the PR head — refusing on an unread value"; exit 3; } +[ -n "$head" ] || { echo "FAIL could not read the PR head"; exit 3; } echo "head at this moment : $head" -# --- 1. the approval must name THIS commit ------------------------------------ -appr=$(gh api "repos/$REPO/pulls/$PR/reviews" \ - --jq '[.[]|select(.state=="APPROVED")|.commit_id]|last' 2>/dev/null | tr -d '" \n') -if [ -z "$appr" ] || [ "$appr" = "null" ]; then +# --- 1. the approval must name THIS commit, and nothing may have superseded it - +api revs '[.[]|{s:.state,c:.commit_id,t:.submitted_at}]|tostring' \ + "repos/$REPO/pulls/$PR/reviews" --paginate || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } +appr=$(printf '%s' "$revs" | python3 -c ' +import json,sys +r=json.loads(sys.stdin.read().replace("][",",")) +a=[x for x in r if x["s"]=="APPROVED"] +print(a[-1]["c"] if a else "")') +last_cr=$(printf '%s' "$revs" | python3 -c ' +import json,sys +r=json.loads(sys.stdin.read().replace("][",",")) +a=[x for x in r if x["s"]=="APPROVED"] +c=[x for x in r if x["s"]=="CHANGES_REQUESTED"] +# A change request submitted AFTER the newest approval still stands. +print(c[-1]["t"] if c and (not a or c[-1]["t"] > a[-1]["t"]) else "")') + +if [ -z "$appr" ]; then echo "FAIL no APPROVED review found"; fail=1 elif [ "$appr" != "$head" ]; then echo "FAIL the approval names $appr, the branch is at $head" @@ -40,21 +77,37 @@ elif [ "$appr" != "$head" ]; then else echo "OK approved SHA == head" fi +if [ -n "$last_cr" ]; then + echo "FAIL a CHANGES_REQUESTED ($last_cr) is newer than the newest approval"; fail=1 +fi -# --- 2. no check may be failing ----------------------------------------------- -runs=$(gh api "repos/$REPO/commits/$head/check-runs" --jq '.check_runs|length' 2>/dev/null | tr -d ' \n') -if [ -z "$runs" ] || [ "$runs" = "0" ]; then - # An empty check list is not a green one. Refuse rather than read silence as success. - echo "FAIL no check runs reported for $head — cannot distinguish 'all green' from 'never ran'" - fail=1 +# --- 2. no check-run and no commit STATUS may be failing ---------------------- +# These are two different APIs. check-runs covers GitHub Actions; the Status API +# covers everything else, and a red status is invisible to the first one. +api total '.total_count' "repos/$REPO/commits/$head/check-runs" \ + || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } +if [ "${total:-0}" -eq 0 ]; then + echo "FAIL no check runs for $head — 'all green' and 'never ran' are not the same reading"; fail=1 +else + api bad '[.check_runs[]|select(.conclusion=="failure" or .conclusion=="timed_out" or .conclusion=="cancelled" or .conclusion=="action_required")|.name]|join(", ")' \ + "repos/$REPO/commits/$head/check-runs" --paginate \ + || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } + api pend '[.check_runs[]|select(.status!="completed")|.name]|join(", ")' \ + "repos/$REPO/commits/$head/check-runs" --paginate \ + || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } + [ -n "$bad" ] && { echo "FAIL failing checks: $bad"; fail=1; } + [ -n "$pend" ] && { echo "FAIL still running: $pend"; fail=1; } + [ -z "$bad" ] && [ -z "$pend" ] && echo "OK $total check runs, none failing or pending" +fi + +api st '.state' "repos/$REPO/commits/$head/status" \ + || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } +api nst '.statuses|length' "repos/$REPO/commits/$head/status" \ + || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } +if [ "$nst" -gt 0 ] && [ "$st" != "success" ]; then + echo "FAIL commit status is '$st' across $nst status(es) — a different API from check-runs"; fail=1 else - bad=$(gh api "repos/$REPO/commits/$head/check-runs" \ - --jq '[.check_runs[]|select(.conclusion=="failure" or .conclusion=="timed_out" or .conclusion=="cancelled")|.name]|join(", ")' 2>/dev/null | tr -d '"') - pend=$(gh api "repos/$REPO/commits/$head/check-runs" \ - --jq '[.check_runs[]|select(.status!="completed")|.name]|join(", ")' 2>/dev/null | tr -d '"') - if [ -n "$bad" ]; then echo "FAIL failing checks: $bad"; fail=1; fi - if [ -n "$pend" ]; then echo "FAIL still running: $pend"; fail=1; fi - [ -z "$bad" ] && [ -z "$pend" ] && echo "OK $runs check runs, none failing or pending" + echo "OK commit statuses: $nst reported, state=$st" fi [ "$fail" -eq 0 ] && echo "PREFLIGHT PASS — safe to merge $PR at $head" || echo "PREFLIGHT FAIL — do not merge $PR" From 5a12a0a6422a21f71c53ef5b1be0e912df8399b6 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 21:01:39 +0700 Subject: [PATCH 03/14] =?UTF-8?q?fix(ci):=20the=20required=20check=20could?= =?UTF-8?q?=20never=20go=20green=20=E2=80=94=20it=20counted=20itself?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As wired, the preflight job could not pass. Three reasons, and the third is the one I should have caught before proposing it as a required check: * a fresh PR has no approval yet — review comes after CI * its sibling checks are still running while it runs * it counted ITSELF: as a check run it is `in_progress`, so it read its own name as pending and failed on it; once red, it read its own failure as a failing check and stayed red. Verified on chain rather than argued: on cb0af21d, `preflight` was the ONLY failure among eleven check runs, and its own log read `FAIL still running: ..., preflight, ...`. Codex named it; pr-daemon reproduced it and pointed out that as a required check this would have blocked every merge in the repository, permanently. --ci reports the two TRANSIENT conditions without failing on them, and the run drops its own name from both the failing and pending lists. What still fails in both modes is what is never transient: an approval naming a DIFFERENT sha (#408) and a check that actually failed (#400/#405). SELF_NAME must equal the check-run name GitHub gives the job, which is the JOB id (`preflight`), not the step name. A wrong value silently restores the self-reference, so the control is on that variable alone: same PR, same run, SELF_NAME=not-the-job-name -> FAIL failing checks: preflight exit 1 SELF_NAME=preflight -> OK 12 check runs, none failing An earlier attempt at that control printed nothing — my grep did not match the line the wrong-name run actually emits — and an inconclusive control is not a control, so it was redone rather than counted. --ci on #410 (approved at head, siblings finished) PASSES, which is the case that proves the check is satisfiable at all. Strict-mode regression unchanged: #400 FAIL, #405 FAIL, #408 FAIL, #398 PASS. The approval leg needs no relaxing and already earned itself: running in production, unprompted, it caught #408's defect occurring on this very PR — `the approval names 60028332…, the branch is at cb0af21d…`. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- .github/workflows/merge-preflight.yml | 24 +++++++++++---- scripts/merge-preflight.sh | 42 ++++++++++++++++++++++----- 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/.github/workflows/merge-preflight.yml b/.github/workflows/merge-preflight.yml index 03d0f162..c6586b32 100644 --- a/.github/workflows/merge-preflight.yml +++ b/.github/workflows/merge-preflight.yml @@ -30,12 +30,26 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - # Its own checks are still running while it runs, so a red "still running" - # here is expected mid-CI and clears on the last push once everything - # settles. What it catches immediately and permanently is the approval - # naming a different SHA than the branch — the #408 shape. + # --ci, because a required check has to be able to go green. Without it + # this job can NEVER pass: a fresh PR has no approval yet, its sibling + # checks are still running while it runs, and it counted ITSELF — first as + # a pending check, then, once red, as a failing one. Verified on chain: on + # cb0af21d `preflight` was the ONLY failure among eleven runs, and its own + # log read `FAIL still running: ..., preflight, ...`. + # + # SELF_NAME must equal the check-run name GitHub gives this job, which is + # the JOB id (`preflight`), not the step name. Getting it wrong silently + # restores the self-reference — it would look configured and behave as it + # did before. + # + # --ci reports the two TRANSIENT conditions (no approval yet, siblings + # still running) without failing on them, and still fails on what is never + # transient: an approval naming a DIFFERENT sha (#408) and a check that + # actually failed (#400/#405). Re-approval re-runs this via + # pull_request_review, so the approval leg recovers without a push. - name: Approval names this SHA, and nothing is failing env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} REPO: ${{ github.repository }} - run: ./scripts/merge-preflight.sh ${{ github.event.pull_request.number }} + SELF_NAME: preflight + run: ./scripts/merge-preflight.sh --ci ${{ github.event.pull_request.number }} diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh index fb68e317..48cebfdc 100755 --- a/scripts/merge-preflight.sh +++ b/scripts/merge-preflight.sh @@ -21,7 +21,22 @@ # Exit 0 = safe to merge. Anything else = do not merge. # ============================================================================= set -uo pipefail -PR="${1:?usage: merge-preflight.sh }" +# --ci downgrades the two conditions that are TRANSIENT during a PR's life: +# * no approval yet — every PR starts unapproved; review comes after CI +# * sibling checks still running — this job runs alongside them +# In --ci mode those are reported and do not fail, so the job can reach green. +# What still fails in BOTH modes is what is never transient: an approval that +# names a DIFFERENT sha (the #408 shape) and a check that actually FAILED +# (the #400/#405 shape). +# +# It also stops counting ITSELF. Run as a check-run, it is `in_progress`, so it +# read its own name as a pending check and failed on it — and once red it read +# its own failure as "a failing check" and stayed red. Verified on chain: on +# cb0af21d, `preflight` was the ONLY failure among eleven runs. +CI_MODE=0 +if [ "${1:-}" = "--ci" ]; then CI_MODE=1; shift; fi +SELF_NAME="${SELF_NAME:-preflight}" +PR="${1:?usage: merge-preflight.sh [--ci] }" REPO="${REPO:-AAStarCommunity/SuperPaymaster}" fail=0 @@ -69,7 +84,11 @@ c=[x for x in r if x["s"]=="CHANGES_REQUESTED"] print(c[-1]["t"] if c and (not a or c[-1]["t"] > a[-1]["t"]) else "")') if [ -z "$appr" ]; then - echo "FAIL no APPROVED review found"; fail=1 + if [ "$CI_MODE" -eq 1 ]; then + echo "INFO no approval yet — transient, not failed in --ci" + else + echo "FAIL no APPROVED review found"; fail=1 + fi elif [ "$appr" != "$head" ]; then echo "FAIL the approval names $appr, the branch is at $head" echo " An approval is a statement about a SHA. Merging takes a branch." @@ -89,15 +108,24 @@ api total '.total_count' "repos/$REPO/commits/$head/check-runs" \ if [ "${total:-0}" -eq 0 ]; then echo "FAIL no check runs for $head — 'all green' and 'never ran' are not the same reading"; fail=1 else - api bad '[.check_runs[]|select(.conclusion=="failure" or .conclusion=="timed_out" or .conclusion=="cancelled" or .conclusion=="action_required")|.name]|join(", ")' \ + api bad '[.check_runs[]|select(.conclusion=="failure" or .conclusion=="timed_out" or .conclusion=="cancelled" or .conclusion=="action_required")|.name]|join("\n")' \ "repos/$REPO/commits/$head/check-runs" --paginate \ || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } - api pend '[.check_runs[]|select(.status!="completed")|.name]|join(", ")' \ + api pend '[.check_runs[]|select(.status!="completed")|.name]|join("\n")' \ "repos/$REPO/commits/$head/check-runs" --paginate \ || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } - [ -n "$bad" ] && { echo "FAIL failing checks: $bad"; fail=1; } - [ -n "$pend" ] && { echo "FAIL still running: $pend"; fail=1; } - [ -z "$bad" ] && [ -z "$pend" ] && echo "OK $total check runs, none failing or pending" + # Drop our own run from both lists before judging them. + bad=$(printf '%s\n' "$bad" | grep -vxF "$SELF_NAME" | grep -v '^$' | paste -sd, -) + pend=$(printf '%s\n' "$pend" | grep -vxF "$SELF_NAME" | grep -v '^$' | paste -sd, -) + [ -n "$bad" ] && { echo "FAIL failing checks: $bad"; fail=1; } + if [ -n "$pend" ]; then + if [ "$CI_MODE" -eq 1 ]; then + echo "INFO still running (transient, not failed in --ci): $pend" + else + echo "FAIL still running: $pend"; fail=1 + fi + fi + [ -z "$bad" ] && [ -z "$pend" ] && echo "OK $total check runs, none failing or pending (excluding $SELF_NAME)" fi api st '.state' "repos/$REPO/commits/$head/status" \ From e67b7c327a0dd50a01f87d7167fd21f1f3c81ee3 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 21:06:52 +0700 Subject: [PATCH 04/14] fix(ci): assert SELF_NAME against the real check runs instead of trusting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SELF_NAME was a hand-maintained string that had to equal the check-run name GitHub assigns. It equals the job id here only because the job sets no `name:` — add one, or rename the job, and it silently stops matching, every exclusion stops excluding, and the self-reference returns looking configured. Raised by pr-daemon on #412. It now defaults to $GITHUB_JOB, so the common case keeps no hand-written string, and --ci ASSERTS the value against the head's actual check-run names before using it. The data was already fetched; this is three lines and it converts a silent misconfiguration into a loud one. SELF_NAME=typo-name -> FAIL SELF_NAME='typo-name' is not among this commit's check runs. (and lists them) SELF_NAME=preflight -> OK SELF_NAME='preflight' matches a real check run "Configured correctly" and "impossible to misconfigure" are different properties, and the second is the one this PR exists to argue for. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- scripts/merge-preflight.sh | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh index 48cebfdc..94bc61a6 100755 --- a/scripts/merge-preflight.sh +++ b/scripts/merge-preflight.sh @@ -35,7 +35,12 @@ set -uo pipefail # cb0af21d, `preflight` was the ONLY failure among eleven runs. CI_MODE=0 if [ "${1:-}" = "--ci" ]; then CI_MODE=1; shift; fi -SELF_NAME="${SELF_NAME:-preflight}" +# Defaults to the job id GitHub exports, so the common case needs no hand-kept +# string at all. Whatever it ends up as, --ci ASSERTS it below against the real +# check-run names: "configured correctly" and "impossible to misconfigure" are +# different properties, and this gate exists to insist on the second. Raised by +# pr-daemon on #412. +SELF_NAME="${SELF_NAME:-${GITHUB_JOB:-preflight}}" PR="${1:?usage: merge-preflight.sh [--ci] }" REPO="${REPO:-AAStarCommunity/SuperPaymaster}" fail=0 @@ -114,6 +119,27 @@ else api pend '[.check_runs[]|select(.status!="completed")|.name]|join("\n")' \ "repos/$REPO/commits/$head/check-runs" --paginate \ || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } + # In CI, this run MUST be among the check-run names, because it IS one. If it + # is not, SELF_NAME does not match the check-run GitHub created — someone added + # a `name:` to the job, or renamed it — and every exclusion below silently + # stops excluding anything. That is the self-reference coming back looking + # configured. Fail loudly instead. + if [ "$CI_MODE" -eq 1 ]; then + api names '[.check_runs[].name]|join("\n")' \ + "repos/$REPO/commits/$head/check-runs" --paginate \ + || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } + if ! printf '%s\n' "$names" | grep -qxF "$SELF_NAME"; then + echo "FAIL SELF_NAME='$SELF_NAME' is not among this commit's check runs." + echo " It must equal the check-run name GitHub gives this job (the job" + echo " id, unless the job sets \`name:\`). A mismatch silently restores" + echo " the self-reference this flag exists to prevent." + echo " check runs here: $(printf '%s' "$names" | paste -sd, -)" + fail=1 + else + echo "OK SELF_NAME='$SELF_NAME' matches a real check run" + fi + fi + # Drop our own run from both lists before judging them. bad=$(printf '%s\n' "$bad" | grep -vxF "$SELF_NAME" | grep -v '^$' | paste -sd, -) pend=$(printf '%s\n' "$pend" | grep -vxF "$SELF_NAME" | grep -v '^$' | paste -sd, -) From 29a9e4b71639007a65f883d0fd39c8f3dab17507 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 21:12:36 +0700 Subject: [PATCH 05/14] fix(ci): derive this run's identity, do not assert its name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The assertion added last commit checked that SELF_NAME appeared among the head's check-run names. That proves a run BY THAT NAME exists — not that it is this one. This very head carried TWO check runs named `preflight`, from two pushes, so the assertion was already satisfiable by something other than the current job. Found by Codex. A check run's details_url is .../actions/runs//job/, so under Actions the name can be DERIVED from the run id instead of configured. That removes the misconfiguration rather than shouting about it: rename the job, add a `name:`, and it still resolves to whatever GitHub actually called this run. The workflow no longer passes a name at all. GITHUB_RUN_ID=33517544569, SELF_NAME=deliberately-wrong -> OK self identified ... -> 'preflight' (the wrong name is ignored) -> OK 11 check runs, none failing or pending (excluding preflight) GITHUB_RUN_ID=999999999 -> FAIL no check run on belongs to GITHUB_RUN_ID=999999999 The first probe is the one that matters: passing a deliberately wrong name and still resolving correctly is what distinguishes "derived" from "asserted". Outside Actions there is no run id, so it falls back to matching by name and says so in the output, naming it as the weaker check rather than letting it read the same as the strong one. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- .github/workflows/merge-preflight.yml | 11 ++++---- scripts/merge-preflight.sh | 40 ++++++++++++++++++++------- 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/.github/workflows/merge-preflight.yml b/.github/workflows/merge-preflight.yml index c6586b32..d27f98f1 100644 --- a/.github/workflows/merge-preflight.yml +++ b/.github/workflows/merge-preflight.yml @@ -37,10 +37,12 @@ jobs: # cb0af21d `preflight` was the ONLY failure among eleven runs, and its own # log read `FAIL still running: ..., preflight, ...`. # - # SELF_NAME must equal the check-run name GitHub gives this job, which is - # the JOB id (`preflight`), not the step name. Getting it wrong silently - # restores the self-reference — it would look configured and behave as it - # did before. + # No SELF_NAME here on purpose. The run DERIVES its own check-run name from + # GITHUB_RUN_ID (a check run's details_url is .../runs//job/), so + # renaming the job or adding a `name:` cannot desynchronise it. An earlier + # version passed the name in and asserted it existed — which proves a run + # by that name exists, not that it is this one. This head carried TWO runs + # named `preflight`. # # --ci reports the two TRANSIENT conditions (no approval yet, siblings # still running) without failing on them, and still fails on what is never @@ -51,5 +53,4 @@ jobs: env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} REPO: ${{ github.repository }} - SELF_NAME: preflight run: ./scripts/merge-preflight.sh --ci ${{ github.event.pull_request.number }} diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh index 94bc61a6..3203fbe6 100755 --- a/scripts/merge-preflight.sh +++ b/scripts/merge-preflight.sh @@ -119,24 +119,44 @@ else api pend '[.check_runs[]|select(.status!="completed")|.name]|join("\n")' \ "repos/$REPO/commits/$head/check-runs" --paginate \ || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } - # In CI, this run MUST be among the check-run names, because it IS one. If it - # is not, SELF_NAME does not match the check-run GitHub created — someone added - # a `name:` to the job, or renamed it — and every exclusion below silently - # stops excluding anything. That is the self-reference coming back looking - # configured. Fail loudly instead. - if [ "$CI_MODE" -eq 1 ]; then + # Identify THIS run, do not merely look for its name. The previous version + # asserted that SELF_NAME appeared among the head's check-run names — which + # proves a run with that name exists, not that it is mine. This head carried + # TWO check runs named `preflight` from two pushes, so presence was already + # satisfiable by something other than the current job. Found by Codex. + # + # A check run's `details_url` is .../actions/runs//job/, so + # in Actions the name can be DERIVED from the run id rather than configured. + # Deriving it removes the misconfiguration instead of shouting about it: rename + # the job, add a `name:`, and this still resolves to whatever GitHub actually + # called it. + if [ "$CI_MODE" -eq 1 ] && [ -n "${GITHUB_RUN_ID:-}" ]; then + api mine "[.check_runs[]|select(.details_url|test(\"/runs/${GITHUB_RUN_ID}/\"))|.name]|join(\"\\n\")" \ + "repos/$REPO/commits/$head/check-runs" --paginate \ + || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } + if [ -z "$mine" ]; then + echo "FAIL no check run on $head belongs to GITHUB_RUN_ID=$GITHUB_RUN_ID." + echo " Cannot identify this job's own check run, so it cannot exclude" + echo " itself, so every judgement below would be self-poisoned." + fail=1 + else + SELF_NAME="$mine" + echo "OK self identified from GITHUB_RUN_ID=$GITHUB_RUN_ID -> '$SELF_NAME'" + fi + elif [ "$CI_MODE" -eq 1 ]; then + # Outside Actions there is no run id to key on. Fall back to the name, and + # say plainly that this is the weaker check — it establishes that A run by + # that name exists, not that it is this one. api names '[.check_runs[].name]|join("\n")' \ "repos/$REPO/commits/$head/check-runs" --paginate \ || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } if ! printf '%s\n' "$names" | grep -qxF "$SELF_NAME"; then echo "FAIL SELF_NAME='$SELF_NAME' is not among this commit's check runs." - echo " It must equal the check-run name GitHub gives this job (the job" - echo " id, unless the job sets \`name:\`). A mismatch silently restores" - echo " the self-reference this flag exists to prevent." echo " check runs here: $(printf '%s' "$names" | paste -sd, -)" fail=1 else - echo "OK SELF_NAME='$SELF_NAME' matches a real check run" + echo "INFO no GITHUB_RUN_ID; matched SELF_NAME='$SELF_NAME' by NAME only" + echo " (weaker: proves a run by that name exists, not that it is this one)" fi fi From 2831c1bba2e1a9693706bc097432732fc76b319b Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 21:19:00 +0700 Subject: [PATCH 06/14] fix(ci): GITHUB_RUN_ID names the workflow run, not this job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lookup matched check runs whose details_url contains /runs//, which is EVERY job in that workflow run. With one job the match is unique by accident, not by construction: add a second job and SELF_NAME silently becomes a multi-line string that excludes nothing. Found by Codex. Not hypothetical in this repo. On this head one run id owns four check runs — Stage 1, Stage 2, Stage 3 and the gate self-test all share one — and another owns two. The single-job case is the exception here, not the rule. The match must now be unique. Zero matches and more than one both fail, and the multi-match message lists the names so the operator can see what collided: 33518175929 (preflight alone) OK self identified -> 'preflight' 33518175901 (four Stage jobs) FAIL owns 4 check runs, listed 999999999 (owns nothing) FAIL no check run belongs to it That listing also fixed a real bug in itself: `printf '%s\n' $mine` unquoted word-split "Stage 2 — forge test + fuzz" into six lines, i.e. it garbled exactly when an operator needed to read it. Also records why the EXCLUSION stays name-based while the IDENTIFICATION is now run-id based — a deliberate widening straight after a deliberate narrowing, which otherwise reads like a bug to fix. Excluding by run id would keep a superseded failing run of this job in `bad` for ever: red once, red always. That is the self-holding trap from two rounds ago in different clothes. Raised by pr-daemon. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- scripts/merge-preflight.sh | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh index 3203fbe6..1b6f39d4 100755 --- a/scripts/merge-preflight.sh +++ b/scripts/merge-preflight.sh @@ -134,14 +134,29 @@ else api mine "[.check_runs[]|select(.details_url|test(\"/runs/${GITHUB_RUN_ID}/\"))|.name]|join(\"\\n\")" \ "repos/$REPO/commits/$head/check-runs" --paginate \ || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } + n_mine=$(printf '%s\n' "$mine" | grep -c '^..*$') if [ -z "$mine" ]; then echo "FAIL no check run on $head belongs to GITHUB_RUN_ID=$GITHUB_RUN_ID." echo " Cannot identify this job's own check run, so it cannot exclude" echo " itself, so every judgement below would be self-poisoned." fail=1 + elif [ "$n_mine" -ne 1 ]; then + # GITHUB_RUN_ID names the WORKFLOW RUN, and every job in it shares that id + # in its details_url. With one job the match is unique by accident, not by + # construction — add a second job here and this silently becomes a + # multi-line SELF_NAME that excludes nothing. Refuse rather than guess + # which of them is me. Found by Codex. + echo "FAIL GITHUB_RUN_ID=$GITHUB_RUN_ID owns $n_mine check runs on this head:" + # Quoted and line-oriented: check-run names contain spaces ("Stage 2 — + # forge test + fuzz"), and an unquoted expansion word-splits them into + # nonsense exactly when the operator most needs to read the list. + printf '%s\n' "$mine" | sed 's/^/ /' + echo " That id identifies the workflow RUN, not this JOB. Make the" + echo " exclusion job-precise before this workflow grows a second job." + fail=1 else SELF_NAME="$mine" - echo "OK self identified from GITHUB_RUN_ID=$GITHUB_RUN_ID -> '$SELF_NAME'" + echo "OK self identified from GITHUB_RUN_ID=$GITHUB_RUN_ID -> '$SELF_NAME' (sole job in the run)" fi elif [ "$CI_MODE" -eq 1 ]; then # Outside Actions there is no run id to key on. Fall back to the name, and @@ -160,7 +175,14 @@ else fi fi - # Drop our own run from both lists before judging them. + # Drop our own run by NAME, deliberately, even though the identification just + # above is by run id. Excluding by run id would keep a SUPERSEDED failing run + # of this same job in `bad` for ever — which is the self-holding trap from two + # rounds ago wearing different clothes: red once, red always. Name-exclusion + # drops every run of this job, including the stale failures a re-push replaces. + # This is a deliberate widening immediately after a deliberate narrowing, so it + # is written down: the identification must be precise, the exclusion must not. + # Raised by pr-daemon. bad=$(printf '%s\n' "$bad" | grep -vxF "$SELF_NAME" | grep -v '^$' | paste -sd, -) pend=$(printf '%s\n' "$pend" | grep -vxF "$SELF_NAME" | grep -v '^$' | paste -sd, -) [ -n "$bad" ] && { echo "FAIL failing checks: $bad"; fail=1; } From 35cb323a0914f41f16bf00d34480dcdf9bf17114 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 21:28:44 +0700 Subject: [PATCH 07/14] =?UTF-8?q?fix(ci):=20the=20approval=20leg=20cannot?= =?UTF-8?q?=20be=20a=20check=20run=20=E2=80=94=20a=20superseded=20red=20ne?= =?UTF-8?q?ver=20clears?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Making preflight required blocked its own PR, and the reason matters more than the fix. At PUSH time no approval can name the new head, so the approval leg fails on every push and leaves a RED check run there. GitHub's rollup then stays FAILURE even after a later run of the same check succeeds, because a superseded failure is still counted. Measured on 2831c1bb: preflight failure@14:20 and success@14:22, reviewDecision=APPROVED, rollup=FAILURE, mergeStateStatus=BLOCKED. That is the "red once, red always" trap I closed inside the script two rounds ago, reappearing one layer up where the script cannot reach it. The leg is NOT dropped. It is moved to where it holds without a race: branch protection `dismiss_stale_reviews`, now enabled, retracts the approval the instant the branch moves. Same property — an approval names a SHA — enforced by GitHub, with no check run to re-run. Strict mode (no --ci) still fails on it, so the pre-merge command keeps the belt; only the CI job treats it as advisory. --ci, approval == head PREFLIGHT PASS (the case that was BLOCKED) strict, #408 FAIL the approval names b327086b…, branch at 4b5084e7… Branch protection now: dismiss_stale_reviews true, approvals 1, required checks ["preflight"], enforce_admins true — read back, not assumed. Two probes were discarded getting here: both used a GITHUB_RUN_ID belonging to an earlier head, so they reported "no check run belongs to this id" — the correct answer to a question I did not mean to ask. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- scripts/merge-preflight.sh | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh index 1b6f39d4..25449af8 100755 --- a/scripts/merge-preflight.sh +++ b/scripts/merge-preflight.sh @@ -97,7 +97,20 @@ if [ -z "$appr" ]; then elif [ "$appr" != "$head" ]; then echo "FAIL the approval names $appr, the branch is at $head" echo " An approval is a statement about a SHA. Merging takes a branch." - fail=1 + # Transient in --ci for the same reason "no approval yet" is: at PUSH time no + # approval can possibly name the new head. Failing here left a RED check run + # on every head, and a superseded failure keeps GitHub's rollup FAILURE even + # after a later run succeeds — so a required check would need a manual re-run + # on every PR. Observed: 2831c1bb had preflight failure@14:20 and success@14:22 + # and stayed BLOCKED with reviewDecision=APPROVED. + # + # This leg is NOT dropped, it is MOVED to where it can be enforced without a + # race: branch protection's dismiss_stale_reviews retracts the approval the + # moment the branch moves, which is the same property GitHub-side and without a + # check run to re-run. Strict mode (no --ci) still fails here, so the pre-merge + # command keeps the belt. + [ "$CI_MODE" -eq 1 ] && echo " (transient in --ci; enforced by dismiss_stale_reviews instead)" + [ "$CI_MODE" -eq 1 ] || fail=1 else echo "OK approved SHA == head" fi From a62e1e4d353a9da77c9d54f763d858d038853e04 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 21:34:42 +0700 Subject: [PATCH 08/14] fix(ci): verify the guard the approval leg was moved to, and stop over-claiming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, one from each reviewer, both the same shape as the PR's own thesis. 1. Codex: --ci downgraded a stale approval without checking that the guard it was MOVED to is actually on. Relocating a guarantee and not verifying the new home is the mistake one level up from the one this gate exists to catch. Branch protection is not readable with GITHUB_TOKEN (Actions has no `administration` scope), so it checks the OBSERVABLE consequence instead: with dismiss_stale_reviews on, a push retracts the newest review and the API reports it DISMISSED. Newest review DISMISSED == the guard is live. NOT "any stale APPROVED exists" — measured, that rule is wrong. GitHub dismisses only the most recent approval, so after enabling it and pushing, #412 still showed APPROVED at 29a9e4b7, e67b7c32 and 5a12a0a6 while 2831c1bb went DISMISSED. Older approvals linger under a WORKING guard, so their presence proves nothing. I had the wrong rule first and the data rejected it. 2. pr-daemon: the sole required check could go green before the checks it stood in for had finished, and the script printed "safe to merge" while five siblings were still running. Passing and being safe to merge are different claims. The real fix was configuration, not script: main now requires all eight contexts — preflight, Stage 1/2/3, test, abi-docs, the secrets scan and the gate self-test — read back after writing. preflight no longer vouches for checks it does not run. The script no longer says "safe to merge" while siblings are pending; it reports that nothing has failed YET and names what is still running. Behaviour now, on this PR: the newest review is pr-daemon's CHANGES_REQUESTED, not DISMISSED, so the downgrade is correctly REFUSED and says why. That is the check working, not failing. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- scripts/merge-preflight.sh | 45 +++++++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh index 25449af8..1586a997 100755 --- a/scripts/merge-preflight.sh +++ b/scripts/merge-preflight.sh @@ -109,8 +109,37 @@ elif [ "$appr" != "$head" ]; then # moment the branch moves, which is the same property GitHub-side and without a # check run to re-run. Strict mode (no --ci) still fails here, so the pre-merge # command keeps the belt. - [ "$CI_MODE" -eq 1 ] && echo " (transient in --ci; enforced by dismiss_stale_reviews instead)" - [ "$CI_MODE" -eq 1 ] || fail=1 + # Downgrading this in --ci is only sound if the guard it was MOVED to is + # actually on. Verify that instead of assuming it — relocating a guarantee and + # not checking the new home is the same mistake one level up. Found by Codex. + # + # Branch protection is not readable with GITHUB_TOKEN (no `administration` + # scope in Actions), so check the OBSERVABLE consequence: with + # dismiss_stale_reviews on, a push retracts the newest review, which the API + # then reports as DISMISSED. So the NEWEST review being DISMISSED is the guard + # working. + # + # Not "any stale APPROVED exists" — measured, that is wrong: GitHub dismisses + # only the most recent approval, so #412 kept APPROVED rows at 29a9e4b7, + # e67b7c32 and 5a12a0a6 while 2831c1bb went DISMISSED. Older approvals linger + # under a working guard, so their presence proves nothing. + if [ "$CI_MODE" -eq 1 ]; then + newest=$(printf '%s' "$revs" | python3 -c ' +import json,sys +r=json.loads(sys.stdin.read().replace("][",",")) +print(sorted(r,key=lambda x:x["t"])[-1]["s"] if r else "")') + if [ "$newest" = "DISMISSED" ]; then + echo " (transient in --ci: newest review is DISMISSED, so" + echo " dismiss_stale_reviews is live and enforcing this)" + else + echo " and the newest review is '$newest', not DISMISSED — so" + echo " dismiss_stale_reviews is NOT retracting approvals and nothing" + echo " is enforcing approval==head. Not downgrading." + fail=1 + fi + else + fail=1 + fi else echo "OK approved SHA == head" fi @@ -219,5 +248,15 @@ else echo "OK commit statuses: $nst reported, state=$st" fi -[ "$fail" -eq 0 ] && echo "PREFLIGHT PASS — safe to merge $PR at $head" || echo "PREFLIGHT FAIL — do not merge $PR" +# "Passed" and "safe to merge" are different claims. With sibling checks still +# running, this run establishes only that nothing has failed YET — saying safe is +# the same over-claim this whole gate argues against. Raised by pr-daemon. +if [ "$fail" -ne 0 ]; then + echo "PREFLIGHT FAIL — do not merge $PR" +elif [ -n "${pend:-}" ]; then + echo "PREFLIGHT PASS (checks still running: ${pend}) — nothing has failed yet;" + echo " merge is gated by the required contexts, not by this line" +else + echo "PREFLIGHT PASS — safe to merge $PR at $head" +fi exit "$fail" From d1fcfbdd79d1e874fa88c26925a8133e1d61e228 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 21:42:53 +0700 Subject: [PATCH 09/14] fix(ci): a review comment must not redden a required check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CHANGES_REQUESTED leg still failed in --ci, so a reviewer doing normal review turned the required check red — and it STAYED red after the author pushed a fix, because re-approval necessarily comes later. A required check that reports "something is wrong with this commit" when what happened is "a human read it" teaches people to ignore it. Found by Codex. GitHub already enforces it without a check run, verified not assumed: this PR reads reviewDecision=CHANGES_REQUESTED, mergeStateStatus=BLOCKED. Advisory in --ci, still failing in strict mode. That exposed a second defect in the guard-evidence added last commit. "Newest review is DISMISSED" was too narrow a test for "dismiss_stale_reviews is live": a reviewer submitting CHANGES_REQUESTED makes the newest review something else while saying nothing about dismissal, so normal review made the approval leg red too. The evidence is now that the guard has FIRED — any DISMISSED review on this PR. It only matters when the approval is stale, and by then the guard has had a push to act on. Third leg moved out of --ci for one reason: a condition TRANSIENT by construction cannot be a required check. A check run records a moment; a required check demands a steady state. And --ci no longer prints "safe to merge" at all. With legs downgraded to GitHub's enforcement, this run cannot speak for mergeability — it says what it saw and names what decides. --ci PASS, both legs downgraded with the enforcer named strict FAIL on both, unchanged #400 FAIL / #408 FAIL / #398 PASS, unchanged Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- scripts/merge-preflight.sh | 50 +++++++++++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 9 deletions(-) diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh index 1586a997..515ae65e 100755 --- a/scripts/merge-preflight.sh +++ b/scripts/merge-preflight.sh @@ -124,17 +124,23 @@ elif [ "$appr" != "$head" ]; then # e67b7c32 and 5a12a0a6 while 2831c1bb went DISMISSED. Older approvals linger # under a working guard, so their presence proves nothing. if [ "$CI_MODE" -eq 1 ]; then + # "Newest review is DISMISSED" was too narrow: a reviewer submitting + # CHANGES_REQUESTED makes the newest review something else without telling us + # anything about dismissal, so a normal review turned this red. What actually + # evidences the guard is that it has FIRED — any DISMISSED review on this PR. + # It only matters when the approval is stale, and by then the guard has had a + # push to act on. newest=$(printf '%s' "$revs" | python3 -c ' import json,sys r=json.loads(sys.stdin.read().replace("][",",")) -print(sorted(r,key=lambda x:x["t"])[-1]["s"] if r else "")') - if [ "$newest" = "DISMISSED" ]; then - echo " (transient in --ci: newest review is DISMISSED, so" - echo " dismiss_stale_reviews is live and enforcing this)" +print("yes" if any(x["s"]=="DISMISSED" for x in r) else "no")') + if [ "$newest" = "yes" ]; then + echo " (transient in --ci: this PR has a DISMISSED review, so" + echo " dismiss_stale_reviews has demonstrably fired and enforces it)" else - echo " and the newest review is '$newest', not DISMISSED — so" - echo " dismiss_stale_reviews is NOT retracting approvals and nothing" - echo " is enforcing approval==head. Not downgrading." + echo " and NO review on this PR has ever been dismissed — so" + echo " dismiss_stale_reviews has never retracted anything and nothing" + echo " is demonstrably enforcing approval==head. Not downgrading." fail=1 fi else @@ -144,7 +150,23 @@ else echo "OK approved SHA == head" fi if [ -n "$last_cr" ]; then - echo "FAIL a CHANGES_REQUESTED ($last_cr) is newer than the newest approval"; fail=1 + echo "FAIL a CHANGES_REQUESTED ($last_cr) is newer than the newest approval" + # Advisory in --ci, and this one is not a subtlety: requesting changes is NORMAL + # REVIEW, not a defect in the commit. Failing on it made a required check go red + # because someone reviewed the PR, and it STAYED red after the author pushed a + # fix, since re-approval necessarily comes later. A required check that reports + # "something is wrong with this commit" when what happened is "a human read it" + # trains people to ignore it. Found by Codex. + # + # GitHub already enforces this without a check run, verified rather than assumed: + # this PR reads reviewDecision=CHANGES_REQUESTED, mergeStateStatus=BLOCKED. So + # the property holds either way; only the reporting changes. + # + # Third leg moved out of --ci for the same reason as the other two: a condition + # that is TRANSIENT by construction cannot be a required check, because a check + # run records a moment and a required check demands a steady state. + [ "$CI_MODE" -eq 1 ] && echo " (transient in --ci; reviewDecision=CHANGES_REQUESTED blocks the merge)" + [ "$CI_MODE" -eq 1 ] || fail=1 fi # --- 2. no check-run and no commit STATUS may be failing ---------------------- @@ -257,6 +279,16 @@ elif [ -n "${pend:-}" ]; then echo "PREFLIGHT PASS (checks still running: ${pend}) — nothing has failed yet;" echo " merge is gated by the required contexts, not by this line" else - echo "PREFLIGHT PASS — safe to merge $PR at $head" + if [ "$CI_MODE" -eq 1 ]; then + # In --ci this run has DOWNGRADED legs that GitHub enforces instead, so it + # cannot speak for mergeability — reviewDecision and the required contexts + # do. Saying "safe to merge" here would be the same over-claim the whole + # gate argues against, one line from the end. + echo "PREFLIGHT PASS — nothing this job can see has failed at $head" + echo " (mergeability is decided by reviewDecision and the" + echo " required contexts, not by this line)" + else + echo "PREFLIGHT PASS — safe to merge $PR at $head" + fi fi exit "$fail" From 8928ef69f56419b2cef0bc19e5feff213d4c7ad8 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 21:48:12 +0700 Subject: [PATCH 10/14] fix(ci): verify the replacement guard where possible, disclaim it where not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Any DISMISSED review exists" does not prove dismiss_stale_reviews is enabled. A human dismissing a review by hand produces an identical row — measured, both DISMISSED rows on this PR carry full ~3.8KB bodies, indistinguishable from any other review — and the row survives the setting being switched off afterwards. A historical event cannot evidence a current setting. Found by Codex. That was the second unsound inference in as many rounds; the first, "newest review is DISMISSED", broke as soon as a reviewer submitted CHANGES_REQUESTED. Two attempts at inferring a config from its side effects, both wrong, is the signal to stop inferring. Strict mode reads the setting directly — its token can — and says what it found: dismiss_stale_reviews=true (pushes retract approvals) dismiss_stale_reviews=false AND ... — nothing enforces this at all. [probed] --ci cannot: GITHUB_TOKEN has no `administration` scope. So it states plainly that THIS RUN HAS NOT CONFIRMED IT and points at the run that does, rather than manufacturing a substitute reading. An honest gap beats a fabricated check — which is the same argument as everything else in this PR, applied to itself. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- scripts/merge-preflight.sh | 57 +++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 32 deletions(-) diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh index 515ae65e..46f7a0d6 100755 --- a/scripts/merge-preflight.sh +++ b/scripts/merge-preflight.sh @@ -109,42 +109,35 @@ elif [ "$appr" != "$head" ]; then # moment the branch moves, which is the same property GitHub-side and without a # check run to re-run. Strict mode (no --ci) still fails here, so the pre-merge # command keeps the belt. - # Downgrading this in --ci is only sound if the guard it was MOVED to is - # actually on. Verify that instead of assuming it — relocating a guarantee and - # not checking the new home is the same mistake one level up. Found by Codex. + # This leg was MOVED to branch protection's dismiss_stale_reviews. Verify that + # where it CAN be verified, and say so plainly where it cannot — rather than + # inferring it from something that only correlates. # - # Branch protection is not readable with GITHUB_TOKEN (no `administration` - # scope in Actions), so check the OBSERVABLE consequence: with - # dismiss_stale_reviews on, a push retracts the newest review, which the API - # then reports as DISMISSED. So the NEWEST review being DISMISSED is the guard - # working. - # - # Not "any stale APPROVED exists" — measured, that is wrong: GitHub dismisses - # only the most recent approval, so #412 kept APPROVED rows at 29a9e4b7, - # e67b7c32 and 5a12a0a6 while 2831c1bb went DISMISSED. Older approvals linger - # under a working guard, so their presence proves nothing. + # Two inferences were tried and both are unsound. "Newest review is DISMISSED" + # breaks the moment a reviewer submits CHANGES_REQUESTED. "Any DISMISSED review + # exists" is worse: a human dismissing a review by hand produces an identical + # row (measured — both DISMISSED rows on this PR carry full ~3.8KB bodies, same + # as any review), and the row survives the setting being turned off afterwards. + # A historical event cannot evidence a current setting. Found by Codex. if [ "$CI_MODE" -eq 1 ]; then - # "Newest review is DISMISSED" was too narrow: a reviewer submitting - # CHANGES_REQUESTED makes the newest review something else without telling us - # anything about dismissal, so a normal review turned this red. What actually - # evidences the guard is that it has FIRED — any DISMISSED review on this PR. - # It only matters when the approval is stale, and by then the guard has had a - # push to act on. - newest=$(printf '%s' "$revs" | python3 -c ' -import json,sys -r=json.loads(sys.stdin.read().replace("][",",")) -print("yes" if any(x["s"]=="DISMISSED" for x in r) else "no")') - if [ "$newest" = "yes" ]; then - echo " (transient in --ci: this PR has a DISMISSED review, so" - echo " dismiss_stale_reviews has demonstrably fired and enforces it)" - else - echo " and NO review on this PR has ever been dismissed — so" - echo " dismiss_stale_reviews has never retracted anything and nothing" - echo " is demonstrably enforcing approval==head. Not downgrading." - fail=1 - fi + # GITHUB_TOKEN has no `administration` scope, so a job cannot read branch + # protection. Do not manufacture a substitute: report the leg, name what is + # supposed to enforce it, and state that this run did NOT confirm it. + echo " (not enforced by this job. dismiss_stale_reviews is supposed to" + echo " enforce it; a GitHub Actions token cannot read branch protection," + echo " so THIS RUN HAS NOT CONFIRMED THAT. The strict pre-merge run does.)" else fail=1 + # Strict mode runs with a token that can read it, so check the guarantee has + # a home instead of trusting that someone left it there. + if dsr=$(gh api "repos/$REPO/branches/main/protection" \ + --jq '.required_pull_request_reviews.dismiss_stale_reviews' 2>/dev/null); then + [ "$dsr" = "true" ] \ + && echo " (dismiss_stale_reviews=true on main, so pushes retract approvals)" \ + || echo " AND dismiss_stale_reviews=$dsr on main — nothing enforces this at all." + else + echo " (could not read branch protection; not claiming it is configured)" + fi fi else echo "OK approved SHA == head" From 316e1bbbdbc563b56d421c78cc19ad9529b1f215 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 21:52:03 +0700 Subject: [PATCH 11/14] fix(ci): the report must stop calling itself the gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --ci passes on a stale approval — the exact #408 condition this work started from. That is defensible (dismiss_stale_reviews and reviewDecision enforce it) but not while the job is a REQUIRED CHECK named `preflight`, because a green tick then claims the approval legs were verified here. They are not; two of the three legs this job cannot even read from a GITHUB_TOKEN. Found by Codex. So the over-claim is removed rather than argued with: * `preflight` is no longer a required context. main now requires the seven checks that actually run something — Stage 1/2/3, test, abi-docs, the secrets scan, the gate self-test — read back after writing. * the job is `merge-preflight-report`, and --ci prints REPORT ONLY with "this is NOT a merge gate" and where each unverified leg is enforced instead. * the job still runs, which was the point of adding it: the script is exercised rather than merely present, which was pr-daemon's original finding. The refusal now lives where it can hold: branch protection for the two review legs, seven required contexts for the checks leg, and the strict pre-merge run of this same script for a human or agent about to press merge. Nothing pretends to cover what it cannot see. --ci REPORT ONLY — ... NOT a merge gate ... strict #408 FAIL, #398 PASS, unchanged Six rounds on this file, and every one moved the same seam: the distance between what a check examines and what a green tick appears to establish. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- .github/workflows/merge-preflight.yml | 14 +++++++++++--- scripts/merge-preflight.sh | 7 ++++--- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/.github/workflows/merge-preflight.yml b/.github/workflows/merge-preflight.yml index d27f98f1..22f5acf9 100644 --- a/.github/workflows/merge-preflight.yml +++ b/.github/workflows/merge-preflight.yml @@ -9,9 +9,17 @@ # # Both legs are observable at push time: the approval-vs-head comparison is why # it re-reads the head on every push rather than trusting an earlier reading. -# Make this a REQUIRED check to turn it from advice into a refusal. +# NOT a required check, deliberately. Its three legs are each enforced by GitHub +# itself — required contexts for the checks, dismiss_stale_reviews for +# approval==head, reviewDecision for CHANGES_REQUESTED — and two of them this job +# cannot even verify from a GITHUB_TOKEN. Listing it as required would have made +# a green tick claim the approval legs were checked here. They are not. +# +# It runs to REPORT, and to make the script exercised rather than merely present +# (which was its original gap). The refusal lives in branch protection and in the +# strict pre-merge run of the same script. # ============================================================================= -name: merge-preflight +name: merge-preflight-report on: pull_request: @@ -26,7 +34,7 @@ permissions: statuses: read jobs: - preflight: + preflight-report: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh index 46f7a0d6..be15a71e 100755 --- a/scripts/merge-preflight.sh +++ b/scripts/merge-preflight.sh @@ -277,9 +277,10 @@ else # cannot speak for mergeability — reviewDecision and the required contexts # do. Saying "safe to merge" here would be the same over-claim the whole # gate argues against, one line from the end. - echo "PREFLIGHT PASS — nothing this job can see has failed at $head" - echo " (mergeability is decided by reviewDecision and the" - echo " required contexts, not by this line)" + echo "REPORT ONLY — nothing this job can see has failed at $head." + echo " This is NOT a merge gate: the approval legs above are" + echo " enforced by dismiss_stale_reviews and reviewDecision and" + echo " are NOT verified here. Run without --ci before merging." else echo "PREFLIGHT PASS — safe to merge $PR at $head" fi From 17e378ec10beadb6465ba8129cbb63098dbd6f26 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 21:56:28 +0700 Subject: [PATCH 12/14] fix(ci): report the review decision, do not assert which one blocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment justifying the --ci downgrade named reviewDecision=CHANGES_REQUESTED as the blocker. That state varies: at d1fcfbdd this PR read REVIEW_REQUIRED, with the approval count doing the blocking, and minutes later CHANGES_REQUESTED. Both readings are correct at their moment, which is exactly why a comment must not freeze one of them — the behaviour stayed safe while the stated reason was wrong half the time. Raised by pr-daemon. It now asks: `gh pr view --json reviewDecision` and prints the live value. Also: strict mode read branch protection from a hardcoded `main`. A PR targeting a release branch would have had a setting reported that does not govern its merge — the right value from the wrong branch, which reads identically to the right answer. Uses the PR's own baseRefName. --ci (transient in --ci; GitHub reports reviewDecision=CHANGES_REQUESTED) strict (dismiss_stale_reviews=true on main, so pushes retract approvals) Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- scripts/merge-preflight.sh | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh index be15a71e..0b07eb4f 100755 --- a/scripts/merge-preflight.sh +++ b/scripts/merge-preflight.sh @@ -130,11 +130,15 @@ elif [ "$appr" != "$head" ]; then fail=1 # Strict mode runs with a token that can read it, so check the guarantee has # a home instead of trusting that someone left it there. - if dsr=$(gh api "repos/$REPO/branches/main/protection" \ + # The PR's own base, not a hardcoded main — this script is run against PRs + # targeting release branches too, and reading the wrong branch's protection + # would report a setting that does not govern this merge. + base=$(gh pr view "$PR" --repo "$REPO" --json baseRefName -q '.baseRefName' 2>/dev/null | tr -d ' \n') + if dsr=$(gh api "repos/$REPO/branches/${base:-main}/protection" \ --jq '.required_pull_request_reviews.dismiss_stale_reviews' 2>/dev/null); then [ "$dsr" = "true" ] \ - && echo " (dismiss_stale_reviews=true on main, so pushes retract approvals)" \ - || echo " AND dismiss_stale_reviews=$dsr on main — nothing enforces this at all." + && echo " (dismiss_stale_reviews=true on ${base:-main}, so pushes retract approvals)" \ + || echo " AND dismiss_stale_reviews=$dsr on ${base:-main} — nothing enforces this at all." else echo " (could not read branch protection; not claiming it is configured)" fi @@ -158,7 +162,15 @@ if [ -n "$last_cr" ]; then # Third leg moved out of --ci for the same reason as the other two: a condition # that is TRANSIENT by construction cannot be a required check, because a check # run records a moment and a required check demands a steady state. - [ "$CI_MODE" -eq 1 ] && echo " (transient in --ci; reviewDecision=CHANGES_REQUESTED blocks the merge)" + # Do not ASSERT which review state is blocking — it varies. At d1fcfbdd this + # PR read REVIEW_REQUIRED (approval count was the blocker) and minutes later + # CHANGES_REQUESTED (the CR was). Naming one of them in a comment made the + # justification wrong half the time even though the behaviour was safe. Ask. + # Raised by pr-daemon. + if [ "$CI_MODE" -eq 1 ]; then + rd=$(gh pr view "$PR" --repo "$REPO" --json reviewDecision -q '.reviewDecision' 2>/dev/null | tr -d ' \n') + echo " (transient in --ci; GitHub reports reviewDecision=${rd:-})" + fi [ "$CI_MODE" -eq 1 ] || fail=1 fi From c1e8b154291fa639f79aa2e232bf5900e41dcde5 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 21:59:34 +0700 Subject: [PATCH 13/14] fix(ci): an unreadable base must not fall back to main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `${base:-main}` substituted a guess for a value it failed to read — the same fail-open this script refuses everywhere else. For a PR targeting a release branch it would have reported main's dismiss_stale_reviews as though it governed that merge: the right value from the wrong branch, which reads exactly like the right answer. Found by Codex. It now says it could not read the base and reports nothing, rather than reporting something about a branch that may not be involved. Probed by forcing base empty, since the obvious probe — pointing REPO at a nonexistent repository — exits several steps earlier on "could not read the PR head" and never reaches this branch. That was the correct refusal to a question I did not mean to ask, and it is the fourth probe today that answered the wrong leg. base readable (dismiss_stale_reviews=true on main, so pushes retract approvals) base empty (could not read this PR's base branch; not reporting a dismiss_stale_reviews value that might govern a different branch) Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- scripts/merge-preflight.sh | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh index 0b07eb4f..816d9bfc 100755 --- a/scripts/merge-preflight.sh +++ b/scripts/merge-preflight.sh @@ -134,11 +134,18 @@ elif [ "$appr" != "$head" ]; then # targeting release branches too, and reading the wrong branch's protection # would report a setting that does not govern this merge. base=$(gh pr view "$PR" --repo "$REPO" --json baseRefName -q '.baseRefName' 2>/dev/null | tr -d ' \n') - if dsr=$(gh api "repos/$REPO/branches/${base:-main}/protection" \ + if [ -z "$base" ]; then + # Do NOT fall back to main. Substituting a guess for an unread value is the + # fail-open this script refuses everywhere else: it would report main's + # setting for a PR that may target a release branch, and the right value + # from the wrong branch reads exactly like the right answer. Found by Codex. + echo " (could not read this PR's base branch; not reporting a" + echo " dismiss_stale_reviews value that might govern a different branch)" + elif dsr=$(gh api "repos/$REPO/branches/$base/protection" \ --jq '.required_pull_request_reviews.dismiss_stale_reviews' 2>/dev/null); then [ "$dsr" = "true" ] \ - && echo " (dismiss_stale_reviews=true on ${base:-main}, so pushes retract approvals)" \ - || echo " AND dismiss_stale_reviews=$dsr on ${base:-main} — nothing enforces this at all." + && echo " (dismiss_stale_reviews=true on $base, so pushes retract approvals)" \ + || echo " AND dismiss_stale_reviews=$dsr on $base — nothing enforces this at all." else echo " (could not read branch protection; not claiming it is configured)" fi From 2d308038fd3dc3271874e87d84370e64f37ef83c Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Tue, 1 Sep 2026 22:06:01 +0700 Subject: [PATCH 14/14] fix(ci): the rename desynchronised strict mode's fallback name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renaming the job to `preflight-report` left `${GITHUB_JOB:-preflight}` behind. Under Actions the run-id lookup overrides it, so CI was unaffected — but STRICT mode has no run id, so the literal is what it uses, and grep -vxF matches whole lines. A stale value excludes nothing, so the pre-merge run counted ITSELF as a failing check and refused the merge. The path designated as the actual gate was the only one broken by the rename. Found by pr-daemon. Fixed, and made self-checking rather than trusting the literal: strict mode now asserts SELF_NAME names a real check run on the head, so the next rename fails loudly instead of turning the gate against itself. default (preflight-report) excluded; failing list holds only the real failure SELF_NAME=preflight (stale) FAIL SELF_NAME matches no check run on this head FAIL failing checks: preflight-report, ... i.e. both symptoms of the rename, named Same shape as the SELF_NAME assertion added for --ci two rounds ago; strict mode had been left without it because at the time its name was correct. A guard that only exists on the path that happens to be right is not a guard. Unrelated and surfaced by this run: `Stage 3 — Slither static scan` is genuinely failing on this head. Not caused by these changes; reported rather than filtered. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2 --- scripts/merge-preflight.sh | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/scripts/merge-preflight.sh b/scripts/merge-preflight.sh index 816d9bfc..bc887afa 100755 --- a/scripts/merge-preflight.sh +++ b/scripts/merge-preflight.sh @@ -40,7 +40,14 @@ if [ "${1:-}" = "--ci" ]; then CI_MODE=1; shift; fi # check-run names: "configured correctly" and "impossible to misconfigure" are # different properties, and this gate exists to insist on the second. Raised by # pr-daemon on #412. -SELF_NAME="${SELF_NAME:-${GITHUB_JOB:-preflight}}" +# The literal is the JOB ID in .github/workflows/merge-preflight.yml. Renaming the +# job there and not here desynchronises them silently: grep -vxF matches whole +# lines, so a stale value excludes nothing and the run counts ITSELF as failing or +# pending. That already happened once — the job became `preflight-report` while +# this still said `preflight`. Under Actions GITHUB_JOB supplies it and the run-id +# lookup overrides it anyway; this literal only bites the STRICT pre-merge run, +# which is the path designated as the actual gate. Raised by pr-daemon. +SELF_NAME="${SELF_NAME:-${GITHUB_JOB:-preflight-report}}" PR="${1:?usage: merge-preflight.sh [--ci] }" REPO="${REPO:-AAStarCommunity/SuperPaymaster}" fail=0 @@ -251,6 +258,21 @@ else fi fi + # Strict mode has no GITHUB_RUN_ID, so SELF_NAME is a literal here. Check it + # names something real before relying on it: a stale literal silently excludes + # nothing, which is how a rename turns this gate against itself. + if [ "$CI_MODE" -ne 1 ]; then + api allnames '[.check_runs[].name]|join("\n")' \ + "repos/$REPO/commits/$head/check-runs" --paginate \ + || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; } + if ! printf '%s\n' "$allnames" | grep -qxF "$SELF_NAME"; then + echo "FAIL SELF_NAME='$SELF_NAME' matches no check run on this head." + echo " It must equal the job id in merge-preflight.yml. A stale value" + echo " excludes nothing and this run then counts itself." + fail=1 + fi + fi + # Drop our own run by NAME, deliberately, even though the identification just # above is by run id. Excluding by run id would keep a SUPERSEDED failing run # of this same job in `bad` for ever — which is the self-holding trap from two