fix(ci): strict merge-preflight refused every PR into an unprotected base

.github/workflows/preflight-selftest.yml

@@ -0,0 +1,29 @@
+# Offline self-test for the STRICT mode of scripts/merge-preflight.sh.
+#
+# merge-preflight.yml only ever runs the script with --ci, and the
+# required-checks leg runs only in strict mode, so before this job CI had zero
+# coverage of the code that decides whether a merge is safe (DSR on #452).
+# The suite replaces `gh` with a fixture server and never touches the network.
+#
+# No paths filter on purpose: a filtered workflow does not appear at all on PRs
+# it skips, and "did not run" must not be mistaken for "passed".
+name: preflight-selftest
+
+on:
+  pull_request:
+  push:
+    branches: [main]
+
+permissions:
+  contents: read
+
+jobs:
+  preflight-strict-fixtures:
+    name: preflight strict-mode fixtures
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v7
+      - name: Strict-mode fixture suite (offline)
+        run: |
+          jq --version
+          python3 scripts/test_merge_preflight_strict.py

scripts/merge-preflight.sh

@@ -335,12 +335,21 @@ 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 allcheck '[.check_runs[].name]|join("\n")' \
+  # A name containing a newline would forge a boundary in the newline-joined
+  # set the required-contexts leg compares against (Codex round 3): refuse it.
+  api allcheck '[.check_runs[].name]|if any(.[]; type!="string" or test("[[:cntrl:]]")) then error("control character in check name") else join("\n") end' \
       "repos/$REPO/commits/$head/check-runs" --paginate \
       || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; }
-  base_for_req=$(gh pr view "$PR" --repo "$REPO" --json baseRefName -q '.baseRefName' 2>/dev/null | tr -d ' \n')
+  base_for_req=$(gh pr view "$PR" --repo "$REPO" --json baseRefName -q '.baseRefName' 2>/dev/null | tr -d ' \n') \
+      || base_for_req=""   # a failed lookup that printed something is still unread
 
-  api bad '[.check_runs[]|select(.conclusion=="failure" or .conclusion=="timed_out" or .conclusion=="cancelled" or .conclusion=="action_required")|.name]|join("\n")' \
+  # Run records for the required-checks allowlist (strict leg below).
+  api runsj '[.check_runs[]|{n:.name,a:.app.id,s:.status,c:.conclusion,i:.id}]|tojson' \
+      "repos/$REPO/commits/$head/check-runs" --paginate \
+      || { echo "PREFLIGHT FAIL — do not merge $PR"; exit 4; }
+  # `stale` is a legal GitHub conclusion for a run abandoned before completing;
+  # it is not success and must not read as quiet (DSR on #452).
+  api bad '[.check_runs[]|select(.conclusion=="failure" or .conclusion=="timed_out" or .conclusion=="cancelled" or .conclusion=="action_required" or .conclusion=="stale")|.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("\n")' \
@@ -466,7 +475,17 @@ else
     # return value and not the side effect, so an unreadable base still failed the
     # run while printing "not claiming". Same shape as every other mismatch today:
     # the verdict arriving by a path other than the one being read.
-    if [ -z "$base_for_req" ]; then
+    # Branch names are path segments in every endpoint below and must be
+    # URL-encoded: `feat/x#y` is a legal ref, and unencoded the request goes to
+    # `feat/x` (the fragment is dropped) - a different branch whose protection
+    # would then be reported as this PR's. Found by Codex. Encoding failure
+    # leaves base_enc empty, which the first branch below refuses - including a
+    # jq that printed the value and then exited non-zero (Codex round 2).
+    base_enc=""
+    if [ -n "$base_for_req" ]; then
+      base_enc=$(jq -rn --arg b "$base_for_req" '$b|@uri' 2>/dev/null) || base_enc=""
+    fi
+    if [ -z "$base_for_req" ] || [ -z "$base_enc" ]; then
       # No `:-main` fallback. That silent substitution was removed three commits
       # ago and the reasoning is still on line ~143 of this file; reinstating it
       # here would report a different branch's configuration as this PR's.
@@ -476,49 +495,108 @@ else
       # strictest. Found by Codex.
       echo "FAIL  base branch unreadable; cannot check required contexts"
       fail=1
-    elif prot=$(gh api "repos/$REPO/branches/$base_for_req/protection" 2>/dev/null) \
-         && [ "$(printf '%s' "$prot" | jq -r 'has("enforce_admins")' 2>/dev/null)" = "true" ]; then
-      # TWO steps, because `// []` alone cannot tell "this branch requires
-      # nothing" from "I could not see what it requires". A partial response, a
-      # permission problem, or an error object all leave the field absent, and
-      # `// []` turned every one of them into "requires nothing" — which PASSES.
-      # That silently converted a fail-closed path into a fail-open one, in the
-      # commit that was fixing a misdiagnosis. Found by Codex.
+    else
+      # The required set is the UNION of what classic branch protection and
+      # repository rulesets require; either can require checks without the
+      # other (Codex round 3). Each source is read fail-closed and VALIDATED,
+      # not normalized: `// []` on a malformed field reads exactly like
+      # "requires nothing".
+      #
+      # A requirement is {context, app_id}; app_id null = any source. A
+      # requirement is satisfied only by the LATEST run with that name (and,
+      # when app_id is set, from that App) being completed with conclusion
+      # "success" - an allowlist, not a failure blacklist. Presence by name
+      # alone let skipped/neutral/stale runs and same-named runs from another
+      # App count as "reported" (DSR + pr-daemon on #452, rounds 2-4).
       #
-      # So: first prove a real protection object was read (`enforce_admins` is present on every
-      # protection object and on nothing else; `.url` was the first choice and did
-      # NOT work — the required_signatures sub-endpoint returns {url, enabled} and
-      # sailed straight through the guard), and only
-      # then treat a missing required_status_checks as a genuine zero.
-      reqctx=$(printf '%s' "$prot" | jq -r '(.required_status_checks.contexts // [])|join("\n")' 2>/dev/null)
-      # `// []` above matters. Without it, a branch WITH protection but NO required
-      # status checks makes jq error on null ("Cannot iterate over null", rc 5), gh
-      # returns 1, and this lands in the unreadable branch — reporting "could not
-      # read" for a branch that simply requires nothing, so strict could never pass
-      # there. Verified with jq directly, both ways. Raised by pr-daemon.
-      if [ -z "$reqctx" ]; then
-        echo "INFO  $base_for_req requires no status checks; nothing to verify reported"
+      # Names must contain a non-space and no control character: a NUL is
+      # stripped by bash command substitution, and the old newline-joined
+      # comparison let a newline forge a boundary (Codex rounds 3-4).
+      JQ_DEFS='def name_ok: type=="string" and test("\\S") and (test("[[:cntrl:]]")|not);
+        def app_ok: . == null or (type=="number" and . > 0 and . == floor);
+        def req($c; $a): if ($c|name_ok) and ($a|app_ok) then {context:$c, app_id:$a} else error("unread") end;'
+      src_ok=1; classic="[]"; classic_desc=""; rsets="[]"
+      if prot=$(gh api "repos/$REPO/branches/$base_enc/protection" 2>/dev/null); then
+        # A real protection object: enforce_admins is an object and url a
+        # string (`.url` alone is not enough - the required_signatures
+        # sub-endpoint returns {url, enabled}). required_status_checks null is
+        # a genuine zero; present but malformed - contexts missing/null/false,
+        # checks not an array of objects, a bad name or app_id - is unread.
+        classic=$(printf '%s' "$prot" | jq -ec "$JQ_DEFS"'
+          if type!="object" or (.enforce_admins|type)!="object" or (.url|type)!="string" then error("unread") else
+          .required_status_checks as $r
+          | if $r == null then []
+            elif ($r|type)!="object" or ($r.contexts|type)!="array" then error("unread")
+            else [ $r.contexts[] | req(.; null) ]
+                 + ( if ($r|has("checks")) then $r.checks else [] end
+                     | if type=="array" and all(.[]; type=="object") then [ .[] | req(.context; .app_id) ] else error("unread") end )
+            end end' 2>/dev/null) && classic_desc="protected" || src_ok=0
       else
-      missing=""
-      while IFS= read -r c; do
-        [ -z "$c" ] && continue
-        printf '%s\n' "$allcheck" | grep -qxF "$c" || missing="${missing:+$missing, }$c"
-      done <<< "$reqctx"
-      if [ -n "$missing" ]; then
-        echo "FAIL  required context(s) never reported on this head: $missing"
-        echo "      A required check that did not run is ABSENT, not passing."
-        echo "      Usually a paths filter: the workflow was not triggered by these"
-        echo "      files. GitHub blocks on it; re-push or widen the filter."
+        # 404 "Branch not protected" has the same shape as "no permission".
+        # Accept "no classic protection" only on a positive read of the branch
+        # object: a boolean protected == false. tostring because jq -e exits 1
+        # when the result IS false (caught by the positive controls).
+        unprot=$(gh api "repos/$REPO/branches/$base_enc" 2>/dev/null) \
+          && unprot=$(printf '%s' "$unprot" | jq -er 'if type=="object" and (.protected|type)=="boolean" then (.protected|tostring) else error("unread") end' 2>/dev/null) \
+          && [ "$unprot" = "false" ] && classic_desc="unprotected" || src_ok=0
+      fi
+      if [ "$src_ok" -eq 1 ]; then
+        # Rulesets, every page (>= 1). Each rule must be an object with a
+        # non-empty string type; each required_status_checks rule must carry a
+        # readable list of {context, integration_id?}.
+        rsets=$(gh api --paginate --slurp "repos/$REPO/rules/branches/$base_enc" 2>/dev/null) \
+          && rsets=$(printf '%s' "$rsets" | jq -ec "$JQ_DEFS"'
+            if type=="array" and length>0 and all(.[]; type=="array") then add else error("unread") end
+            | if all(.[]; type=="object" and (.type|type)=="string" and (.type|length)>0) then . else error("unread") end
+            | [ .[] | select(.type=="required_status_checks")
+                | .parameters.required_status_checks
+                | if type=="array" and all(.[]; type=="object") then .[] else error("unread") end
+                | req(.context; .integration_id) ]' 2>/dev/null) || src_ok=0
+      fi
+      # Every check run on this head, for the allowlist below. Pages arrive
+      # as one JSON array per line; each record must be well-formed.
+      if [ "$src_ok" -eq 1 ]; then
+        runs=$(printf '%s\n' "$runsj" | jq -ecs '
+          if all(.[]; type=="array") then add // [] else error("unread") end
+          | if all(.[]; type=="object" and (.n|type)=="string" and (.i|type)=="number" and (.s|type)=="string"
+                       and ((.a|type)=="number" or .a==null) and ((.c|type)=="string" or .c==null))
+            then . else error("unread") end' 2>/dev/null) || src_ok=0
+      fi
+      if [ "$src_ok" -ne 1 ]; then
+        echo "FAIL  could not read $base_for_req's required checks (classic protection, rulesets or check runs)."
+        echo "      Not proceeding on an unread value: whether every required check"
+        echo "      succeeded is exactly what this leg exists to establish."
+        fail=1
+      elif ! verdict=$(jq -ern --argjson a "$classic" --argjson b "$rsets" --argjson runs "$runs" '
+          # union; a specific-App requirement supersedes an any-App one for the same context
+          ($a + $b | unique) as $all
+          | [ $all[] as $r
+              | select($r.app_id != null or ([ $all[] | select(.context == $r.context and .app_id != null) ] | length) == 0)
+              | $r ] as $req
+          | [ $req[] as $r
+              | ([ $runs[] | select(.n == $r.context and ($r.app_id == null or .a == $r.app_id)) ] | max_by(.i)) as $l
+              | if $l == null then "\($r.context) [absent\(if $r.app_id != null then " from app \($r.app_id)" else "" end)]"
+                elif $l.s != "completed" then "\($r.context) [status=\($l.s)]"
+                elif $l.c != "success" then "\($r.context) [conclusion=\($l.c)]"
+                else empty end ] as $bad
+          | "\($req|length)\t\($bad|join("; "))"' 2>/dev/null); then
+        echo "FAIL  could not evaluate $base_for_req's required checks; refusing on an unread value"
         fail=1
       else
-        echo "OK    all $(printf '%s\n' "$reqctx" | grep -c .) required contexts reported"
+        nreq=${verdict%%	*}; unmet=${verdict#*	}
+        if [ "$nreq" = "0" ]; then
+          echo "INFO  $base_for_req ($classic_desc, no ruleset check rule) requires no status checks;"
+          echo "      nothing to verify reported"
+        elif [ -n "$unmet" ]; then
+          echo "FAIL  required check(s) not satisfied on this head: $unmet"
+          echo "      Only the latest run of a required check, from the required App,"
+          echo "      completed with conclusion=success counts. A required check that"
+          echo "      did not run is ABSENT, not passing; skipped/neutral/stale are not success."
+          fail=1
+        else
+          echo "OK    all $nreq required checks succeeded ($classic_desc + rulesets)"
+        fi
       fi
-      fi
-    else
-      echo "FAIL  could not read $base_for_req's required contexts."
-      echo "      Not proceeding on an unread value: whether every required check"
-      echo "      reported is exactly what this leg exists to establish."
-      fail=1
     fi
   fi
 fi