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..51c567c 100755 --- a/scripts/merge-pr.sh +++ b/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 --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 " >&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 --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` 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" diff --git a/templates/repo-setup.md b/templates/repo-setup.md index 1ea9972..19ec96a 100644 --- a/templates/repo-setup.md +++ b/templates/repo-setup.md @@ -30,12 +30,30 @@ 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` 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 + 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