merge-pr: handle real-repo merge configs (method, required-checks, approving-review)

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)

scripts/merge-pr.sh

@@ -29,7 +29,30 @@ 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. It discovers
+#                    which checks are REQUIRED from the PR's own status-check rollup, via
+#                    `gh pr checks <pr> --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
+#                    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 +81,10 @@ set -euo pipefail
 usage() {
   echo "usage: $0 <PR#>" >&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 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
 }
 
 if [ "$#" -lt 1 ] || [ -z "${1:-}" ]; then
@@ -161,47 +186,159 @@ if [ "$current_head" != "$reviewed_sha" ]; then
   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").
-# `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.
-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
+# 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
-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
+
+# 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 status-check rollup marks any check REQUIRED. We discover that from the
+# PR's OWN rollup via `gh pr checks <pr> --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.)
+#
+# `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>' 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).
+#
+#   * 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.
+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"
+
+# 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
+
+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 "       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
+  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 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
+  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
+  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
-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
+
+# 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"
+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
 
 # 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.)
+# 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
@@ -213,5 +350,5 @@ fi
 # 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"