From 462acfe5ac400932ec816b6df67992a9705b9c70 Mon Sep 17 00:00:00 2001 From: Yihan Zhu <48186361+yihanzhu@users.noreply.github.com> Date: Sat, 27 Jun 2026 08:02:17 -0400 Subject: [PATCH 1/4] merge-pr: handle real-repo merge configs (method, required-checks, approving-review) scripts/merge-pr.sh broke on common real-repo branch settings, all on the happy path. This makes the mechanical merge gate work against them while preserving every existing safety guard. - Merge method: detect allowed methods via `gh repo view --json squashMergeAllowed,mergeCommitAllowed,rebaseMergeAllowed`; prefer --squash, fall back to --merge then --rebase; refuse if none allowed. --match-head-commit SHA-pin works across all three. - CI gate, required vs optional (operator-approved): when the base branch's protection defines required status checks, gate on exactly those required contexts (a required context that is absent/non-pass blocks; optional checks are informational and never block). When no required checks are defined (the protection endpoint 404s), fall back to the legacy gate (>=1 pass, none failing/pending). Guarantee preserved: never merge with a failing required check; never merge with zero passing checks. - Approving-review protection: pre-check `gh pr view --json reviewDecision`; refuse on REVIEW_REQUIRED with a clear message that the comments-only reviewer never approves, so this protection is incompatible with in-session auto-merge (hand to the human merge gate). Additive. - Docs: templates/repo-setup.md states the supported protection shape (required status checks, NOT require approving review) and notes merged-branch cleanup; README Layout + behavior text synced. Preserves: reviewed head/base SHA marker parsing, refuse-if-head/base-moved, --match-head-commit pin, --repo scoping, unset GH_REPO. Stays #!/usr/bin/env bash, set -euo pipefail, shellcheck -S style clean, +x. Closes #55 Co-Authored-By: Claude Opus 4.8 --- README.md | 10 ++- scripts/merge-pr.sh | 154 +++++++++++++++++++++++++++++++++------- templates/repo-setup.md | 18 ++++- 3 files changed, 153 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 0a24b0b..d8693a3 100644 --- a/README.md +++ b/README.md @@ -96,8 +96,12 @@ exactly one coder launch per cleared issue, one review path, and one revision pa not hand-craft a merge command). `merge-pr.sh` owns the mechanical safety: it reads the reviewed head+base SHAs from the authenticated `codex-review.sh` marker, confirms the PR's current head **and** base still match those (refusing if either moved since the review), - requires ≥1 real passing CI check, and merges **pinned via `--match-head-commit`** — refusing - otherwise. The merge is **scoped to the target repo** (never another repo) and **bound to the + gates on the base branch's **required status checks** (falling back to ≥1 real passing CI + check with none failing when no required checks are defined — optional checks like preview + deploys are informational), refuses a PR that needs an **approving review** + (`reviewDecision=REVIEW_REQUIRED`, since the comments-only reviewer never approves), and + merges with a **repo-permitted method** (squash if allowed) **pinned via + `--match-head-commit`** — refusing otherwise. The merge is **scoped to the target repo** (never another repo) and **bound to the exact head Faber reviewed** — if the head moved, Faber **re-reviews rather than merges** (the script itself refuses a moved head; a head Codex never reviewed is never merged). A later **status/Tracking scan and the brief only surface `merge-ready` PRs (read-only)** — they never @@ -144,7 +148,7 @@ reviewer/manager-review.md Codex manager-reviewer mechanism (issue-as-bus): roun scripts/install.sh Generate the /faber command with a repo-derived path (idempotent) scripts/codex-review.sh Codex reviewer harness: post `codex exec review` to a PR, verbatim (stamps Reviewed-head: marker) scripts/manager-review.sh Codex manager-reviewer harness: debate a proposed issue vs. the north star, post the verdict to the issue verbatim -scripts/merge-pr.sh Safe in-session merge harness: SHA-pin to reviewed head + repo-scope + CI-green, then squash-merge +scripts/merge-pr.sh Safe in-session merge harness: SHA-pin to reviewed head + repo-scope + required-checks gate + review-required refuse, then merge (repo-permitted method) scripts/setup-target-repo.sh Bootstrap a target repo's loop labels (idempotent) templates/faber-command.md Template for the /faber command (path placeholder) templates/target-CLAUDE.md Drop into each target repo (conventions + PR-size rule) diff --git a/scripts/merge-pr.sh b/scripts/merge-pr.sh index fec30b7..e46c6bc 100755 --- a/scripts/merge-pr.sh +++ b/scripts/merge-pr.sh @@ -29,7 +29,22 @@ set -euo pipefail # PR's CURRENT base still equals it immediately before the merge, so a # base that advanced after the review (a different effective diff) is # refused. -# 3. CI-green — it confirms CI is green on the current head before merging. +# 3. CI-green — it confirms CI is green on the current head before merging. When the +# base branch's protection defines REQUIRED status checks, the gate is +# those required contexts (a pending/failing OPTIONAL check — a preview +# deploy, coverage bot, etc. — is informational and does NOT block). When +# no required checks are defined (unprotected / free private repo), it +# falls back to the legacy gate: refuse unless ≥1 check passes and none +# fail/pend. Either way the guarantee holds: never merge with a failing +# REQUIRED check, and never merge with zero passing checks. +# 3b. Review-gate — if the base branch requires ≥1 APPROVING review (the PR's +# reviewDecision is REVIEW_REQUIRED), it refuses: Fabrica's reviewer is +# comments-only and never approves, so that protection is incompatible +# with the in-session auto-merge path — it hands to the human merge gate. +# 3c. Merge-method — it detects the repo's allowed merge methods (squash / merge / rebase) +# and prefers `--squash`, falling back to a permitted method; the +# `--match-head-commit` SHA-pin works across all three. If none is +# allowed it refuses with an actionable message. # # RESIDUAL LIMITATION (base-race, honest): `--match-head-commit` pins ONLY the head — gh # has no `--match-base-commit`, so the merge cannot atomically pin the base server-side the @@ -58,8 +73,10 @@ set -euo pipefail usage() { echo "usage: $0 " >&2 echo " run from within the target repo's clone, after scripts/codex-review.sh on the same PR" >&2 - echo " refuses unless: a Reviewed-head marker exists, the PR head still equals it, and CI is green" >&2 - echo " then squash-merges, pinned to the reviewed SHA (--match-head-commit)" >&2 + echo " refuses unless: a Reviewed-head marker exists, the PR head still equals it, CI is green" >&2 + echo " (required checks if branch protection defines them, else ≥1 pass / no fail), and the" >&2 + echo " PR does not need an approving review (Fabrica's reviewer is comments-only)" >&2 + echo " then merges (squash if allowed, else a permitted method), pinned to the reviewed SHA" >&2 } if [ "$#" -lt 1 ] || [ -z "${1:-}" ]; then @@ -161,36 +178,105 @@ if [ "$current_head" != "$reviewed_sha" ]; then exit 1 fi +# Approving-review protection (additive guard). GitHub's `reviewDecision` is REVIEW_REQUIRED +# when the base branch's protection requires ≥1 approving review and none is present. Fabrica's +# Codex reviewer is COMMENTS-ONLY and never approves, so `gh pr merge` would be rejected +# server-side with no actionable message. Refuse early and clearly: this protection is +# incompatible with the in-session auto-merge path — the PR must go to the human merge gate. +# (APPROVED / CHANGES_REQUESTED / null are not our concern here; the SHA-pin + CI gate below +# are the mechanical safety. We only intercept the REVIEW_REQUIRED dead-end.) +review_decision="$(gh pr view "$pr" --repo "$repo" --json reviewDecision -q .reviewDecision 2>/dev/null || true)" +if [ "$review_decision" = "REVIEW_REQUIRED" ]; then + echo "error: PR #$pr requires an approving review (reviewDecision=REVIEW_REQUIRED); refusing to merge" >&2 + echo " Fabrica's reviewer (codex-review.sh) is comments-only and never approves, so this" >&2 + echo " branch-protection shape is incompatible with the in-session auto-merge path" >&2 + echo " hand this PR to the human merge gate, or switch protection to required status checks" >&2 + echo " (see templates/repo-setup.md > Branch protection)" >&2 + exit 1 +fi + # Confirm CI is green on the current head. `gh pr checks --json` tags each check with a -# `bucket` (pass / fail / pending / skipping / cancel). Green here means BOTH: -# - AT LEAST ONE check is `pass` — something CI actually ran and passed; and -# - NO check is fail/pending/cancel (skipping is tolerated alongside the pass). -# Requiring a real pass closes the all-skipped hole: when every reported check is -# `skipping` (e.g. jobs gated off by `if:` conditions), there is no fail/pending/cancel -# but nothing actually passed — that must NOT count as green. We still refuse on zero -# checks (CI must be the gate, so "no checks" is not "green"). +# `bucket` (pass / fail / pending / skipping / cancel). Which checks GATE the merge depends +# on whether the PR's BASE branch defines REQUIRED status checks in its branch protection: +# +# * REQUIRED checks defined (protected base) — gate on EXACTLY those required contexts: +# - every required context must be present on the head AND in bucket `pass` +# (a missing required context = it hasn't reported yet = not green); and +# - a non-required (optional) check in ANY bucket — a pending preview deploy, a +# failing coverage bot — is INFORMATIONAL and does NOT block. (operator-approved +# semantics change: optional checks no longer stall a genuinely mergeable PR.) +# Guarantee preserved: we never merge with a failing/absent REQUIRED check, and the set +# of required contexts is non-empty here, so ≥1 check passes by construction. +# +# * NO required checks defined (unprotected / free private repo — the protection endpoint +# 404s) — fall back to the LEGACY gate over ALL reported checks: +# - AT LEAST ONE check is `pass` (something CI actually ran and passed); and +# - NO check is fail/pending/cancel (skipping is tolerated alongside the pass); +# - refuse on zero checks (CI is the gate, so "no checks" is not "green"). +# This closes the all-skipped hole: if every check is `skipping`, nothing passed. +# +# Either way: never merge with a failing required check; never merge with zero passing checks. +# # `gh pr checks` exits non-zero when checks aren't all passing (e.g. 8 = pending), so we # capture its output without letting `set -e` abort, then judge the buckets ourselves. We -# request `name` alongside `bucket` so the not-green diagnostic below can name the failing -# checks — `gh --json` returns only requested fields, so omitting `name` would print null. +# request `name` alongside `bucket` so diagnostics can name the offending checks — `gh --json` +# returns only requested fields, so omitting `name` would print null. checks_json="$(gh pr checks "$pr" --repo "$repo" --json name,bucket 2>/dev/null || true)" if [ -z "$checks_json" ] || [ "$(jq 'length' <<<"$checks_json")" -eq 0 ]; then echo "error: no CI checks found on PR #$pr head ($current_head); refusing to merge" >&2 echo " CI is the hard gate — there must be at least one green check" >&2 exit 1 fi -not_green="$(jq -r '[.[] | select(.bucket != "pass" and .bucket != "skipping")] | length' <<<"$checks_json")" -if [ "$not_green" -ne 0 ]; then - echo "error: CI not green on PR #$pr head ($current_head); refusing to merge" >&2 - echo " not-passing checks:" >&2 - jq -r '.[] | select(.bucket != "pass" and .bucket != "skipping") | " - \(.name): \(.bucket)"' <<<"$checks_json" >&2 - exit 1 + +# Determine the REQUIRED status-check contexts from the base branch's protection. The +# endpoint 404s (and `gh api` exits non-zero) when the branch is unprotected OR protected +# without required status checks — both are the "no required checks" fallback. We capture +# the body and exit code separately so `set -e` can't abort on the expected 404, then read +# the `contexts` array (legacy field; the `checks[].context` shape carries the same names). +base_ref="$(gh pr view "$pr" --repo "$repo" --json baseRefName -q .baseRefName)" +required_json="" +if required_json="$(gh api "repos/$repo/branches/$base_ref/protection/required_status_checks" 2>/dev/null)"; then + : +else + required_json="" fi -passing="$(jq -r '[.[] | select(.bucket == "pass")] | length' <<<"$checks_json")" -if [ "$passing" -eq 0 ]; then - echo "error: no passing CI check on PR #$pr head ($current_head); refusing to merge" >&2 - echo " every check is skipped/optional — CI must actually run and pass to be the gate" >&2 - exit 1 +required_contexts="$( + jq -r '([.contexts // []] | flatten) + ([.checks // [] | map(.context)] | flatten) | unique | .[]' \ + <<<"${required_json:-{}}" 2>/dev/null || true +)" + +if [ -n "$required_contexts" ]; then + # Protected base with required checks — gate on those contexts only. + not_green_required="" + while IFS= read -r ctx; do + [ -z "$ctx" ] && continue + bucket="$(jq -r --arg ctx "$ctx" '[.[] | select(.name == $ctx)] | (.[0].bucket // "missing")' <<<"$checks_json")" + if [ "$bucket" != "pass" ]; then + not_green_required+=" - $ctx: $bucket"$'\n' + fi + done <<<"$required_contexts" + if [ -n "$not_green_required" ]; then + echo "error: required CI check(s) not green on PR #$pr head ($current_head); refusing to merge" >&2 + echo " required checks not passing (optional checks are informational and ignored):" >&2 + printf '%s' "$not_green_required" >&2 + exit 1 + fi +else + # No required checks (unprotected / free private repo) — legacy gate over all checks. + not_green="$(jq -r '[.[] | select(.bucket != "pass" and .bucket != "skipping")] | length' <<<"$checks_json")" + if [ "$not_green" -ne 0 ]; then + echo "error: CI not green on PR #$pr head ($current_head); refusing to merge" >&2 + echo " (no required checks defined on base '$base_ref' — gating on all checks)" >&2 + echo " not-passing checks:" >&2 + jq -r '.[] | select(.bucket != "pass" and .bucket != "skipping") | " - \(.name): \(.bucket)"' <<<"$checks_json" >&2 + exit 1 + fi + passing="$(jq -r '[.[] | select(.bucket == "pass")] | length' <<<"$checks_json")" + if [ "$passing" -eq 0 ]; then + echo "error: no passing CI check on PR #$pr head ($current_head); refusing to merge" >&2 + echo " every check is skipped/optional — CI must actually run and pass to be the gate" >&2 + exit 1 + fi fi # Confirm the PR's CURRENT base still equals the reviewed base SHA — done HERE, as the LAST @@ -209,9 +295,27 @@ if [ "$current_base" != "$reviewed_base" ]; then exit 1 fi +# Pick a merge method the repo actually allows. Hardcoding `--squash` fails server-side on +# any repo that disallows squash merges; we detect the permitted methods and PREFER squash +# (the repo's own history convention here), falling back to merge-commit then rebase. The +# `--match-head-commit` SHA-pin below works across all three methods, so the safety guarantee +# is method-independent. If the repo allows NO merge method, refuse with an actionable error. +merge_flags="$(gh repo view "$repo" --json squashMergeAllowed,mergeCommitAllowed,rebaseMergeAllowed)" +if [ "$(jq -r '.squashMergeAllowed' <<<"$merge_flags")" = "true" ]; then + merge_method="--squash" +elif [ "$(jq -r '.mergeCommitAllowed' <<<"$merge_flags")" = "true" ]; then + merge_method="--merge" +elif [ "$(jq -r '.rebaseMergeAllowed' <<<"$merge_flags")" = "true" ]; then + merge_method="--rebase" +else + echo "error: repo $repo allows no merge method (squash/merge/rebase all disabled); refusing" >&2 + echo " enable at least one merge method in the repo's settings, then re-run" >&2 + exit 1 +fi + # All guards passed — merge, pinned to the reviewed SHA. `--match-head-commit` is a # server-side belt-and-suspenders on top of the head==reviewed check above: GitHub itself # refuses the merge if the head isn't exactly this commit, closing the tiny window between # our check and the merge call. -echo "merging PR #$pr (reviewed head $reviewed_sha, base $reviewed_base, CI green) ..." -gh pr merge "$pr" --repo "$repo" --squash --match-head-commit "$reviewed_sha" +echo "merging PR #$pr (reviewed head $reviewed_sha, base $reviewed_base, CI green, method ${merge_method#--}) ..." +gh pr merge "$pr" --repo "$repo" "$merge_method" --match-head-commit "$reviewed_sha" diff --git a/templates/repo-setup.md b/templates/repo-setup.md index 1ea9972..ad09689 100644 --- a/templates/repo-setup.md +++ b/templates/repo-setup.md @@ -30,12 +30,28 @@ It reports per label `matches` / `differs` (which of name/color/description) / ` and exits non-zero if anything is missing or differs (zero if all match). ## 2. Branch protection (main) -- ✅ Require status checks to pass before merging (your CI) — the **hard gate** +The supported protection shape is **required status checks** — that is the gate +`scripts/merge-pr.sh` reads and enforces. +- ✅ Require status checks to pass before merging (your CI) — the **hard gate**. Mark your + CI contexts (lint/test/build) as **required**; `merge-pr.sh` gates on exactly the required + contexts and treats any non-required check (preview deploys, coverage bots) as + informational, so a pending/failing optional check won't stall a mergeable PR. If you + leave the base unprotected (or define no required checks), the script falls back to + requiring ≥1 passing check with none failing/pending. - ✅ Require branches to be up to date before merging +- ⛔️ **Do NOT use "Require a pull request before merging → require approving review."** + Fabrica's reviewer (`scripts/codex-review.sh`) is **comments-only and never approves**, so a + required approving review can never be satisfied by the loop — `merge-pr.sh` detects this + (`reviewDecision=REVIEW_REQUIRED`) and refuses, handing the PR to the human merge gate. + Gate on required **status checks**, not on a required approving review. - ⛔️ **Keep GitHub's native auto-merge button off** — merges run through Faber or the human (both gated on green CI), not a server-side auto-merge trigger. Faber merging a clean, low-risk PR is a deliberate `gh pr merge`, not this checkbox. +**Merged-branch cleanup.** `merge-pr.sh` does not delete the head branch. Either enable the +repo's **"Automatically delete head branches"** setting, or run `gh pr merge … --delete-branch` +manually, so merged feature branches don't accumulate. + ## 3. CI — the loop's hard gate CI must run the **exact commands** you put in this repo's `CLAUDE.md` (its tests / lint/typecheck / build) on every PR. The contract is simple: what the agents are told to From 3c625bf3a2286c65cd27ce7fe3e7e7a129109ffd Mon Sep 17 00:00:00 2001 From: Yihan Zhu <48186361+yihanzhu@users.noreply.github.com> Date: Sat, 27 Jun 2026 08:12:17 -0400 Subject: [PATCH 2/4] fix: URL-encode base branch name in protection lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The branch-protection lookup interpolated $base_ref raw into the REST path. A slashed base name (release/1.0, feature/x) sent its slashes as path separators, so the lookup 404'd and the script wrongly fell back to the legacy all-checks gate — refusing a protected release PR whose required checks pass but an optional check is pending/failing. URL-encode the branch path component with jq's @uri filter (/ → %2F) before the gh api call. The 404/no-protection fallback and the required-context gating are otherwise unchanged. Co-Authored-By: Claude Opus 4.8 --- scripts/merge-pr.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/merge-pr.sh b/scripts/merge-pr.sh index e46c6bc..4f8f27e 100755 --- a/scripts/merge-pr.sh +++ b/scripts/merge-pr.sh @@ -234,8 +234,13 @@ fi # the body and exit code separately so `set -e` can't abort on the expected 404, then read # the `contexts` array (legacy field; the `checks[].context` shape carries the same names). base_ref="$(gh pr view "$pr" --repo "$repo" --json baseRefName -q .baseRefName)" +# URL-encode the branch name for the REST path: slashed names like `release/1.0` or +# `feature/x` carry `/`, which a raw interpolation would send as path separators — the +# lookup would 404 and wrongly fall back to the legacy all-checks gate. `@uri` encodes +# `/` → `%2F` (and other reserved chars). +base_ref_enc="$(jq -rn --arg b "$base_ref" '$b | @uri')" required_json="" -if required_json="$(gh api "repos/$repo/branches/$base_ref/protection/required_status_checks" 2>/dev/null)"; then +if required_json="$(gh api "repos/$repo/branches/$base_ref_enc/protection/required_status_checks" 2>/dev/null)"; then : else required_json="" From 64a73a37436f153f8e04d1a7cc38ffb0943b0dc1 Mon Sep 17 00:00:00 2001 From: Yihan Zhu <48186361+yihanzhu@users.noreply.github.com> Date: Sat, 27 Jun 2026 08:22:55 -0400 Subject: [PATCH 3/4] merge-pr: keep base-race read last; fail closed on indeterminate required-checks discovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review fixes for PR #59 (issue #55): Fix 1 — keep the base-race guard last: the merge-method detection (gh repo view --json squashMergeAllowed,...) was moved BEFORE the final base-race re-check, so the baseRefOid read is once again the LAST API call before gh pr merge. An extra API round-trip after the base read would let another PR land on the base in the gap and merge onto an unreviewed base. Fix 2 — fail CLOSED when required-checks discovery is indeterminate: the required_status_checks lookup now takes the legacy all-checks fallback ONLY on a confirmed HTTP 404 (branch protection / required checks not configured) or a successful response with an empty required-context set. Any other failure (403/5xx/429/network) refuses with an actionable stderr message naming the HTTP cause, rather than misclassifying a protected base as 'no required checks' (which would let an optional pending/failing check block the merge and lie in the diagnostic). gh api's stderr is captured via mktemp and inspected for 'HTTP 404'. The genuine-404 legacy fallback and the jq @uri branch URL-encoding from round-1 are preserved. Header and inline comments updated to reflect the reordering and the fail-closed semantics. Co-Authored-By: Claude Opus 4.8 --- scripts/merge-pr.sh | 109 ++++++++++++++++++++++++++++++++------------ 1 file changed, 79 insertions(+), 30 deletions(-) diff --git a/scripts/merge-pr.sh b/scripts/merge-pr.sh index 4f8f27e..fdd7b88 100755 --- a/scripts/merge-pr.sh +++ b/scripts/merge-pr.sh @@ -33,10 +33,18 @@ set -euo pipefail # base branch's protection defines REQUIRED status checks, the gate is # those required contexts (a pending/failing OPTIONAL check — a preview # deploy, coverage bot, etc. — is informational and does NOT block). When -# no required checks are defined (unprotected / free private repo), it -# falls back to the legacy gate: refuse unless ≥1 check passes and none -# fail/pend. Either way the guarantee holds: never merge with a failing -# REQUIRED check, and never merge with zero passing checks. +# no required checks are defined (CONFIRMED by a genuine HTTP 404 — branch +# unprotected / required checks not configured — or a successful response +# with an empty required-context set), it falls back to the legacy gate: +# refuse unless ≥1 check passes and none fail/pend. If required-checks +# discovery is INDETERMINATE (the endpoint failed for any reason OTHER than +# a genuine 404 — 403 no-read-permission, rate limit, transient 5xx, network +# error), it FAILS CLOSED: refuse with the HTTP cause rather than guess "no +# required checks" (guessing would misclassify a protected base and let an +# optional check block, or merge onto a base whose required checks went +# unverified). Either way the guarantee holds: never merge with a failing +# REQUIRED check, never merge with zero passing checks, and never merge when +# we could not determine the required checks. # 3b. Review-gate — if the base branch requires ≥1 APPROVING review (the PR's # reviewDecision is REVIEW_REQUIRED), it refuses: Fabrica's reviewer is # comments-only and never approves, so that protection is incompatible @@ -228,22 +236,53 @@ if [ -z "$checks_json" ] || [ "$(jq 'length' <<<"$checks_json")" -eq 0 ]; then exit 1 fi -# Determine the REQUIRED status-check contexts from the base branch's protection. The -# endpoint 404s (and `gh api` exits non-zero) when the branch is unprotected OR protected -# without required status checks — both are the "no required checks" fallback. We capture -# the body and exit code separately so `set -e` can't abort on the expected 404, then read -# the `contexts` array (legacy field; the `checks[].context` shape carries the same names). +# Determine the REQUIRED status-check contexts from the base branch's protection, and +# FAIL CLOSED if discovery is indeterminate. The required-checks endpoint returns a genuine +# HTTP 404 only when the branch is unprotected OR protected WITHOUT required status checks — +# THAT (and only that) is the "no required checks" case that takes the legacy all-checks +# fallback. Any OTHER failure (403 = no branch-protection read permission, 429 rate-limit, +# 5xx, network error) means we COULD NOT DETERMINE the required checks; treating that as +# "no required checks" would misclassify a protected base and let an optional pending/failing +# check block the merge (the regression this change fixes) while the diagnostic lies. So we +# capture `gh api`'s stderr + exit code, detect a real `HTTP 404` for the fallback, and +# refuse on anything else. base_ref="$(gh pr view "$pr" --repo "$repo" --json baseRefName -q .baseRefName)" # URL-encode the branch name for the REST path: slashed names like `release/1.0` or # `feature/x` carry `/`, which a raw interpolation would send as path separators — the # lookup would 404 and wrongly fall back to the legacy all-checks gate. `@uri` encodes # `/` → `%2F` (and other reserved chars). base_ref_enc="$(jq -rn --arg b "$base_ref" '$b | @uri')" +# Capture `gh api`'s stderr (where it prints the `(HTTP )` on failure) in a temp file +# so we can distinguish a genuine 404 from any other error. mktemp gives an unpredictable +# path (no `$$` collision/symlink race), and we delete it the moment we've read it back. +required_err="" required_json="" -if required_json="$(gh api "repos/$repo/branches/$base_ref_enc/protection/required_status_checks" 2>/dev/null)"; then - : -else - required_json="" +required_rc=0 +required_errfile="$(mktemp "${TMPDIR:-/tmp}/merge-pr-required-err.XXXXXX")" +required_json="$(gh api "repos/$repo/branches/$base_ref_enc/protection/required_status_checks" 2>"$required_errfile")" || required_rc=$? +required_err="$(cat "$required_errfile" 2>/dev/null || true)" +rm -f "$required_errfile" +if [ "$required_rc" -ne 0 ]; then + # The call failed. Take the legacy fallback ONLY on a confirmed HTTP 404 (branch + # protection / required checks not configured). On any other error (403/5xx/429/network) + # we cannot tell what the required checks are — refuse rather than risk merging onto a + # base whose real required checks we never read. + if printf '%s' "$required_err" | grep -q 'HTTP 404'; then + required_json="" # genuine "no required checks" → legacy fallback below + else + http_code="$(printf '%s' "$required_err" | grep -oE 'HTTP [0-9]{3}' | head -n1)" + [ -z "$http_code" ] && http_code="no HTTP status (network/transport error)" + echo "error: could not determine required status checks for '$base_ref' ($http_code); refusing to merge" >&2 + echo " discovery of the base branch's required checks failed for a reason other than" >&2 + echo " 'not configured' (a genuine 404) — e.g. missing branch-protection read access," >&2 + echo " a rate limit, a transient 5xx, or a network error. Merging blind could land onto" >&2 + echo " a protected base whose required checks were never verified." >&2 + echo " re-run, or grant the gh operator branch-protection read access on '$repo'." >&2 + if [ -n "$required_err" ]; then + echo " gh api error: $(printf '%s' "$required_err" | head -n1)" >&2 + fi + exit 1 + fi fi required_contexts="$( jq -r '([.contexts // []] | flatten) + ([.checks // [] | map(.context)] | flatten) | unique | .[]' \ @@ -267,7 +306,10 @@ if [ -n "$required_contexts" ]; then exit 1 fi else - # No required checks (unprotected / free private repo) — legacy gate over all checks. + # No required checks — confirmed by a genuine 404 (branch unprotected / required checks + # not configured) OR a successful response with an empty required-context set. (An + # indeterminate discovery failure already refused above, so we never reach here blind.) + # Legacy gate over all reported checks. not_green="$(jq -r '[.[] | select(.bucket != "pass" and .bucket != "skipping")] | length' <<<"$checks_json")" if [ "$not_green" -ne 0 ]; then echo "error: CI not green on PR #$pr head ($current_head); refusing to merge" >&2 @@ -284,27 +326,15 @@ else fi fi -# Confirm the PR's CURRENT base still equals the reviewed base SHA — done HERE, as the LAST -# read immediately before the merge, to minimize the base-race window. `--match-head-commit` -# and the head check above only pin the PR head; gh has no `--match-base-commit`, so the base -# cannot be pinned server-side and we must verify it as late as possible. If the base branch -# advanced after the review (in a repo without an up-to-date-branch merge queue), the merge -# would integrate the head with a base Codex never saw — a different effective diff — yet the -# head guard would still pass. Binding the base here closes most of that hole: refuse and ask -# for a re-review. (Residual: a base landing in the gap between this read and the merge below -# is closed only by server-side branch protection / required-up-to-date — see the header.) -current_base="$(gh pr view "$pr" --repo "$repo" --json baseRefOid -q .baseRefOid)" -if [ "$current_base" != "$reviewed_base" ]; then - echo "error: base advanced since review ($reviewed_base -> $current_base); re-review before merging" >&2 - echo " run scripts/codex-review.sh $pr on the current base, then re-run this" >&2 - exit 1 -fi - # Pick a merge method the repo actually allows. Hardcoding `--squash` fails server-side on # any repo that disallows squash merges; we detect the permitted methods and PREFER squash # (the repo's own history convention here), falling back to merge-commit then rebase. The # `--match-head-commit` SHA-pin below works across all three methods, so the safety guarantee # is method-independent. If the repo allows NO merge method, refuse with an actionable error. +# IMPORTANT: this network read (and every other) is done BEFORE the base-race re-check below, +# so the `baseRefOid` read stays the LAST API call before `gh pr merge`. Doing it after the +# base re-check would reopen the base-race window: another PR could land on the base during +# this extra API round-trip, and we'd merge onto a base Codex never reviewed. merge_flags="$(gh repo view "$repo" --json squashMergeAllowed,mergeCommitAllowed,rebaseMergeAllowed)" if [ "$(jq -r '.squashMergeAllowed' <<<"$merge_flags")" = "true" ]; then merge_method="--squash" @@ -318,6 +348,25 @@ else exit 1 fi +# Confirm the PR's CURRENT base still equals the reviewed base SHA — done HERE, as the LAST +# API read immediately before the merge, to minimize the base-race window. Every other network +# read (merge-method detection above, CI checks, review-decision, head re-check) happens +# earlier ON PURPOSE so this `baseRefOid` read is the final call before `gh pr merge` — no API +# round-trip sits between it and the merge. `--match-head-commit` and the head check only pin +# the PR head; gh has no `--match-base-commit`, so the base cannot be pinned server-side and we +# must verify it as late as possible. If the base branch advanced after the review (in a repo +# without an up-to-date-branch merge queue), the merge would integrate the head with a base +# Codex never saw — a different effective diff — yet the head guard would still pass. Binding +# the base here closes most of that hole: refuse and ask for a re-review. (Residual: a base +# landing in the gap between this read and the merge below is closed only by server-side branch +# protection / required-up-to-date — see the header.) +current_base="$(gh pr view "$pr" --repo "$repo" --json baseRefOid -q .baseRefOid)" +if [ "$current_base" != "$reviewed_base" ]; then + echo "error: base advanced since review ($reviewed_base -> $current_base); re-review before merging" >&2 + echo " run scripts/codex-review.sh $pr on the current base, then re-run this" >&2 + exit 1 +fi + # All guards passed — merge, pinned to the reviewed SHA. `--match-head-commit` is a # server-side belt-and-suspenders on top of the head==reviewed check above: GitHub itself # refuses the merge if the head isn't exactly this commit, closing the tiny window between From 3dca01780e9b8e928c565511bd19a4536373a2a9 Mon Sep 17 00:00:00 2001 From: Yihan Zhu <48186361+yihanzhu@users.noreply.github.com> Date: Sat, 27 Jun 2026 08:48:56 -0400 Subject: [PATCH 4/4] fix(merge-pr): discover required checks via PR rollup, not branch protection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The required-check discovery used `gh api .../protection/required_status_checks`, which needs Administration / branch-protection READ permission. A non-admin maintainer who can merge PRs but lacks that permission hit a permission error, and the round-2 fail-closed path then refused even a green PR — breaking a common protected-repo maintainer setup. Replace it with `gh pr checks --required`, which derives "required" from the PR's status-check rollup `isRequired` flag — readable by anyone who can view the PR (no admin permission). Decide from output CONTENT, not exit code: 1. non-empty JSON array -> required checks present: gate on exactly those (refuse unless every entry's bucket is pass; optional checks ignored). 2. benign "no required checks" message (no array) -> legacy fallback over the FULL set: require >=1 pass, zero failing/non-skipped, refuse on zero. 3. neither (genuine auth/network error) -> fail closed: refuse with the error. Removes the now-obsolete `jq @uri` branch URL-encoding (round-1) and the `gh api .../protection/required_status_checks` call with its 404-vs-error fail-closed distinction (round-2). Updates the header/inline comments and templates/repo-setup.md to describe the rollup-based discovery (works for non-admin maintainers). Preserves round-2's other fixes: merge-method detection stays before the final baseRefOid read (the last API call before merge); all `gh pr checks` reads happen before that base read. reviewDecision refuse, SHA-pin guards, --repo-scoping, and unset GH_REPO unchanged. Tested on PR #59 (no required checks): takes the legacy fallback, finds the `ci` check passing, none failing/pending — proceeds, does not fail closed. Closes #55 Co-Authored-By: Claude Opus 4.8 --- scripts/merge-pr.sh | 195 ++++++++++++++++++---------------------- templates/repo-setup.md | 12 +-- 2 files changed, 94 insertions(+), 113 deletions(-) diff --git a/scripts/merge-pr.sh b/scripts/merge-pr.sh index fdd7b88..51c567c 100755 --- a/scripts/merge-pr.sh +++ b/scripts/merge-pr.sh @@ -29,22 +29,22 @@ set -euo pipefail # PR's CURRENT base still equals it immediately before the merge, so a # base that advanced after the review (a different effective diff) is # refused. -# 3. CI-green — it confirms CI is green on the current head before merging. When the -# base branch's protection defines REQUIRED status checks, the gate is -# those required contexts (a pending/failing OPTIONAL check — a preview -# deploy, coverage bot, etc. — is informational and does NOT block). When -# no required checks are defined (CONFIRMED by a genuine HTTP 404 — branch -# unprotected / required checks not configured — or a successful response -# with an empty required-context set), it falls back to the legacy gate: -# refuse unless ≥1 check passes and none fail/pend. If required-checks -# discovery is INDETERMINATE (the endpoint failed for any reason OTHER than -# a genuine 404 — 403 no-read-permission, rate limit, transient 5xx, network -# error), it FAILS CLOSED: refuse with the HTTP cause rather than guess "no -# required checks" (guessing would misclassify a protected base and let an -# optional check block, or merge onto a base whose required checks went -# unverified). Either way the guarantee holds: never merge with a failing -# REQUIRED check, never merge with zero passing checks, and never merge when -# we could not determine the required checks. +# 3. CI-green — it confirms CI is green on the current head before merging. It discovers +# which checks are REQUIRED from the PR's own status-check rollup, via +# `gh pr checks --required` (the rollup's `isRequired` flag) — readable +# by anyone who can view the PR, so NO branch-protection / Administration +# read permission is needed (a non-admin maintainer who can merge but lacks +# that permission is no longer locked out). When the rollup reports REQUIRED +# checks, the gate is exactly those (a pending/failing OPTIONAL check — a +# preview deploy, coverage bot, etc. — is informational and does NOT block). +# When the rollup reports NO required checks (gh prints `no required checks +# reported …` and emits no JSON), it falls back to the legacy gate over the +# FULL check set: refuse unless ≥1 check passes and none fail/pend. If the +# discovery call fails for any OTHER reason (auth/network/transient error — +# neither a JSON array nor the benign no-required message), it FAILS CLOSED: +# refuse with the captured error rather than guess. Either way the guarantee +# holds: never merge with a failing REQUIRED check, never merge with zero +# passing checks, and never merge when we could not determine the checks. # 3b. Review-gate — if the base branch requires ≥1 APPROVING review (the PR's # reviewDecision is REVIEW_REQUIRED), it refuses: Fabrica's reviewer is # comments-only and never approves, so that protection is incompatible @@ -82,7 +82,7 @@ usage() { echo "usage: $0 " >&2 echo " run from within the target repo's clone, after scripts/codex-review.sh on the same PR" >&2 echo " refuses unless: a Reviewed-head marker exists, the PR head still equals it, CI is green" >&2 - echo " (required checks if branch protection defines them, else ≥1 pass / no fail), and the" >&2 + echo " (required checks if the PR rollup reports any, else ≥1 pass / no fail), and the" >&2 echo " PR does not need an approving review (Fabrica's reviewer is comments-only)" >&2 echo " then merges (squash if allowed, else a permitted method), pinned to the reviewed SHA" >&2 } @@ -205,115 +205,82 @@ fi # Confirm CI is green on the current head. `gh pr checks --json` tags each check with a # `bucket` (pass / fail / pending / skipping / cancel). Which checks GATE the merge depends -# on whether the PR's BASE branch defines REQUIRED status checks in its branch protection: +# on whether the PR's status-check rollup marks any check REQUIRED. We discover that from the +# PR's OWN rollup via `gh pr checks --required` (gh derives "required" from each check +# run's `isRequired` flag) — this is readable by ANYONE who can view the PR, so it needs NO +# branch-protection / Administration read permission. (The prior approach read the base +# branch's protection via `gh api .../protection/required_status_checks`, which needs that +# admin-level permission; a non-admin maintainer who could merge but lacked it hit a +# permission error and the fail-closed path then refused even a green PR. The rollup removes +# that requirement.) # -# * REQUIRED checks defined (protected base) — gate on EXACTLY those required contexts: -# - every required context must be present on the head AND in bucket `pass` -# (a missing required context = it hasn't reported yet = not green); and -# - a non-required (optional) check in ANY bucket — a pending preview deploy, a -# failing coverage bot — is INFORMATIONAL and does NOT block. (operator-approved -# semantics change: optional checks no longer stall a genuinely mergeable PR.) -# Guarantee preserved: we never merge with a failing/absent REQUIRED check, and the set -# of required contexts is non-empty here, so ≥1 check passes by construction. +# `gh pr checks --required` behaves (gh 2.92.0): +# * required checks present — exits 0, prints a JSON array of just the required checks. +# * no required checks — exits non-zero, prints NO JSON array, and writes +# `no required checks reported on the '' branch` to +# stderr. This is the benign "unprotected / no required +# checks" case → legacy fallback over the full set. +# * genuine error (auth/net) — exits non-zero with neither a JSON array nor that benign +# message → fail closed. +# `gh pr checks` also exits non-zero merely because checks aren't all green (e.g. pending), +# so we DECIDE FROM THE OUTPUT CONTENT, not the exit code: capture stdout and stderr without +# letting `set -e` abort, then classify. We request `name,state,bucket` so diagnostics can +# name offending checks (`gh --json` returns only requested fields). # -# * NO required checks defined (unprotected / free private repo — the protection endpoint -# 404s) — fall back to the LEGACY gate over ALL reported checks: +# * REQUIRED checks present — gate on EXACTLY those required checks: +# - every required check must be in bucket `pass`; any other bucket (pending preview +# deploy that is required, a failing required test) blocks — name the non-pass ones. +# - a non-required (optional) check in ANY bucket is INFORMATIONAL and does NOT block. +# Guarantee preserved: we never merge with a non-pass REQUIRED check; the required set is +# non-empty here, so ≥1 check passes by construction. +# +# * NO required checks reported — fall back to the LEGACY gate over ALL reported checks: # - AT LEAST ONE check is `pass` (something CI actually ran and passed); and # - NO check is fail/pending/cancel (skipping is tolerated alongside the pass); # - refuse on zero checks (CI is the gate, so "no checks" is not "green"). # This closes the all-skipped hole: if every check is `skipping`, nothing passed. # # Either way: never merge with a failing required check; never merge with zero passing checks. -# -# `gh pr checks` exits non-zero when checks aren't all passing (e.g. 8 = pending), so we -# capture its output without letting `set -e` abort, then judge the buckets ourselves. We -# request `name` alongside `bucket` so diagnostics can name the offending checks — `gh --json` -# returns only requested fields, so omitting `name` would print null. -checks_json="$(gh pr checks "$pr" --repo "$repo" --json name,bucket 2>/dev/null || true)" -if [ -z "$checks_json" ] || [ "$(jq 'length' <<<"$checks_json")" -eq 0 ]; then - echo "error: no CI checks found on PR #$pr head ($current_head); refusing to merge" >&2 - echo " CI is the hard gate — there must be at least one green check" >&2 - exit 1 -fi +req_errfile="$(mktemp "${TMPDIR:-/tmp}/merge-pr-required-err.XXXXXX")" +# `|| true` so the non-zero exit (no-required-checks, or merely-pending checks) does not +# abort under `set -e` — we classify from the OUTPUT CONTENT below, not the exit code. +req_out="$(gh pr checks "$pr" --repo "$repo" --required --json name,state,bucket 2>"$req_errfile" || true)" +req_err="$(cat "$req_errfile" 2>/dev/null || true)" +rm -f "$req_errfile" -# Determine the REQUIRED status-check contexts from the base branch's protection, and -# FAIL CLOSED if discovery is indeterminate. The required-checks endpoint returns a genuine -# HTTP 404 only when the branch is unprotected OR protected WITHOUT required status checks — -# THAT (and only that) is the "no required checks" case that takes the legacy all-checks -# fallback. Any OTHER failure (403 = no branch-protection read permission, 429 rate-limit, -# 5xx, network error) means we COULD NOT DETERMINE the required checks; treating that as -# "no required checks" would misclassify a protected base and let an optional pending/failing -# check block the merge (the regression this change fixes) while the diagnostic lies. So we -# capture `gh api`'s stderr + exit code, detect a real `HTTP 404` for the fallback, and -# refuse on anything else. -base_ref="$(gh pr view "$pr" --repo "$repo" --json baseRefName -q .baseRefName)" -# URL-encode the branch name for the REST path: slashed names like `release/1.0` or -# `feature/x` carry `/`, which a raw interpolation would send as path separators — the -# lookup would 404 and wrongly fall back to the legacy all-checks gate. `@uri` encodes -# `/` → `%2F` (and other reserved chars). -base_ref_enc="$(jq -rn --arg b "$base_ref" '$b | @uri')" -# Capture `gh api`'s stderr (where it prints the `(HTTP )` on failure) in a temp file -# so we can distinguish a genuine 404 from any other error. mktemp gives an unpredictable -# path (no `$$` collision/symlink race), and we delete it the moment we've read it back. -required_err="" -required_json="" -required_rc=0 -required_errfile="$(mktemp "${TMPDIR:-/tmp}/merge-pr-required-err.XXXXXX")" -required_json="$(gh api "repos/$repo/branches/$base_ref_enc/protection/required_status_checks" 2>"$required_errfile")" || required_rc=$? -required_err="$(cat "$required_errfile" 2>/dev/null || true)" -rm -f "$required_errfile" -if [ "$required_rc" -ne 0 ]; then - # The call failed. Take the legacy fallback ONLY on a confirmed HTTP 404 (branch - # protection / required checks not configured). On any other error (403/5xx/429/network) - # we cannot tell what the required checks are — refuse rather than risk merging onto a - # base whose real required checks we never read. - if printf '%s' "$required_err" | grep -q 'HTTP 404'; then - required_json="" # genuine "no required checks" → legacy fallback below - else - http_code="$(printf '%s' "$required_err" | grep -oE 'HTTP [0-9]{3}' | head -n1)" - [ -z "$http_code" ] && http_code="no HTTP status (network/transport error)" - echo "error: could not determine required status checks for '$base_ref' ($http_code); refusing to merge" >&2 - echo " discovery of the base branch's required checks failed for a reason other than" >&2 - echo " 'not configured' (a genuine 404) — e.g. missing branch-protection read access," >&2 - echo " a rate limit, a transient 5xx, or a network error. Merging blind could land onto" >&2 - echo " a protected base whose required checks were never verified." >&2 - echo " re-run, or grant the gh operator branch-protection read access on '$repo'." >&2 - if [ -n "$required_err" ]; then - echo " gh api error: $(printf '%s' "$required_err" | head -n1)" >&2 - fi - exit 1 - fi +# Is req_out a non-empty JSON array (required checks present)? Tolerate the non-zero rc. +req_is_array="false" +if [ -n "$req_out" ] && jq -e 'type == "array" and length > 0' >/dev/null 2>&1 <<<"$req_out"; then + req_is_array="true" fi -required_contexts="$( - jq -r '([.contexts // []] | flatten) + ([.checks // [] | map(.context)] | flatten) | unique | .[]' \ - <<<"${required_json:-{}}" 2>/dev/null || true -)" -if [ -n "$required_contexts" ]; then - # Protected base with required checks — gate on those contexts only. - not_green_required="" - while IFS= read -r ctx; do - [ -z "$ctx" ] && continue - bucket="$(jq -r --arg ctx "$ctx" '[.[] | select(.name == $ctx)] | (.[0].bucket // "missing")' <<<"$checks_json")" - if [ "$bucket" != "pass" ]; then - not_green_required+=" - $ctx: $bucket"$'\n' - fi - done <<<"$required_contexts" +if [ "$req_is_array" = "true" ]; then + # REQUIRED checks present (from the PR rollup) — gate on exactly those. + not_green_required="$( + jq -r '.[] | select(.bucket != "pass") | " - \(.name): \(.bucket)"' <<<"$req_out" + )" + total_required="$(jq -r 'length' <<<"$req_out")" if [ -n "$not_green_required" ]; then echo "error: required CI check(s) not green on PR #$pr head ($current_head); refusing to merge" >&2 - echo " required checks not passing (optional checks are informational and ignored):" >&2 - printf '%s' "$not_green_required" >&2 + echo " gated on the $total_required required check(s) from the PR rollup" >&2 + echo " (optional/non-required checks are informational and ignored):" >&2 + printf '%s\n' "$not_green_required" >&2 + exit 1 + fi + echo "CI gate: all $total_required required check(s) pass (from PR rollup; optional checks ignored)" +elif printf '%s\n%s' "$req_out" "$req_err" | grep -qi 'no required checks'; then + # NO required checks reported by the rollup — the benign case. Legacy gate over the FULL + # check set: ≥1 pass and nothing failing/pending (skipping tolerated), refuse on zero. + checks_json="$(gh pr checks "$pr" --repo "$repo" --json name,bucket 2>/dev/null || true)" + if [ -z "$checks_json" ] || [ "$(jq 'length' <<<"$checks_json")" -eq 0 ]; then + echo "error: no CI checks found on PR #$pr head ($current_head); refusing to merge" >&2 + echo " CI is the hard gate — there must be at least one green check" >&2 exit 1 fi -else - # No required checks — confirmed by a genuine 404 (branch unprotected / required checks - # not configured) OR a successful response with an empty required-context set. (An - # indeterminate discovery failure already refused above, so we never reach here blind.) - # Legacy gate over all reported checks. not_green="$(jq -r '[.[] | select(.bucket != "pass" and .bucket != "skipping")] | length' <<<"$checks_json")" if [ "$not_green" -ne 0 ]; then echo "error: CI not green on PR #$pr head ($current_head); refusing to merge" >&2 - echo " (no required checks defined on base '$base_ref' — gating on all checks)" >&2 + echo " (no required checks reported on PR #$pr — legacy fallback gating on all checks)" >&2 echo " not-passing checks:" >&2 jq -r '.[] | select(.bucket != "pass" and .bucket != "skipping") | " - \(.name): \(.bucket)"' <<<"$checks_json" >&2 exit 1 @@ -324,6 +291,18 @@ else echo " every check is skipped/optional — CI must actually run and pass to be the gate" >&2 exit 1 fi + echo "CI gate: legacy fallback (no required checks reported) — $passing passing, none failing/pending" +else + # Neither a JSON array of required checks nor the benign "no required checks" message — a + # genuine unexpected failure (auth/network/transient). Fail closed: refuse rather than guess. + echo "error: could not determine required status checks for PR #$pr; refusing to merge" >&2 + echo " 'gh pr checks --required' returned neither a list of required checks nor the" >&2 + echo " benign 'no required checks' message — likely an auth, rate-limit, or network" >&2 + echo " error. Merging blind could land with unverified checks. Re-run when resolved." >&2 + if [ -n "$req_err" ]; then + echo " gh error: $(printf '%s' "$req_err" | head -n1)" >&2 + fi + exit 1 fi # Pick a merge method the repo actually allows. Hardcoding `--squash` fails server-side on diff --git a/templates/repo-setup.md b/templates/repo-setup.md index ad09689..19ec96a 100644 --- a/templates/repo-setup.md +++ b/templates/repo-setup.md @@ -33,11 +33,13 @@ and exits non-zero if anything is missing or differs (zero if all match). The supported protection shape is **required status checks** — that is the gate `scripts/merge-pr.sh` reads and enforces. - ✅ Require status checks to pass before merging (your CI) — the **hard gate**. Mark your - CI contexts (lint/test/build) as **required**; `merge-pr.sh` gates on exactly the required - contexts and treats any non-required check (preview deploys, coverage bots) as - informational, so a pending/failing optional check won't stall a mergeable PR. If you - leave the base unprotected (or define no required checks), the script falls back to - requiring ≥1 passing check with none failing/pending. + CI contexts (lint/test/build) as **required**; `merge-pr.sh` discovers the required checks + from the PR's own status-check rollup (`gh pr checks --required`, readable by anyone who can + view the PR — **no branch-protection / admin read access needed**, so a non-admin maintainer + who can merge is supported), gates on exactly those required checks, and treats any + non-required check (preview deploys, coverage bots) as informational, so a pending/failing + optional check won't stall a mergeable PR. If you leave the base unprotected (or define no + required checks), the script falls back to requiring ≥1 passing check with none failing/pending. - ✅ Require branches to be up to date before merging - ⛔️ **Do NOT use "Require a pull request before merging → require approving review."** Fabrica's reviewer (`scripts/codex-review.sh`) is **comments-only and never approves**, so a