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

[yihanzhu]

Closes #55

What changed

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.

1. Merge method (AC Add RESTORE.md — runbook to rebuild the whole team from this repo #1). Detect allowed methods via gh repo view --json squashMergeAllowed,mergeCommitAllowed,rebaseMergeAllowed; prefer --squash, fall back to --merge then --rebase; refuse with an actionable error if none is allowed. --match-head-commit SHA-pin works across all three methods, so the safety guarantee is method-independent.
2. CI gate — required vs optional (AC Add Claude Code GitHub Workflow #2, operator-approved). Determine required contexts from the base branch's protection (gh api repos/{owner}/{repo}/branches/{branch}/protection/required_status_checks). When required checks are defined, gate on exactly those contexts — a required context that is absent or non-pass blocks; any non-required (optional) check is informational and never blocks. When no required checks are defined (endpoint 404s — unprotected / free private repo), fall back to the legacy gate (≥1 pass, none failing/pending, refuse on zero checks).
3. Approving-review protection (AC Add Claude Code GitHub Workflow #3, additive). Pre-check gh pr view --json reviewDecision; on REVIEW_REQUIRED, refuse with a clear message that Fabrica's reviewer is comments-only (never approves), so this protection is incompatible with the in-session auto-merge path — hand to the human merge gate.
4. Docs (AC docs: add RESTORE.md disaster-recovery runbook #4). templates/repo-setup.md now states the supported protection shape (required status checks, NOT "require approving review") and notes merged-branch cleanup (--delete-branch or the repo's auto-delete setting). README Layout line + behavior paragraph synced.

CI-gate semantics change — callout (operator signed off)

This PR changes the CI gate semantics, and the operator has explicitly signed off (per the issue): the gate is now the required status checks when branch protection defines them — a pending/failing optional check (Vercel/Codecov/preview) no longer blocks a genuinely mergeable PR. When no required checks are defined, behavior is unchanged (legacy ≥1-pass / no-fail fallback). Guarantee preserved: never merge with a failing required check; never merge with zero passing checks. Refuse messages still name the offending checks.

Preserved guards (unchanged)

Reviewed head/base SHA marker parsing; refuse-if-head-moved; refuse-if-base-moved; --match-head-commit pin; --repo-scoping + unset GH_REPO; ≥1-real-CI-pass (in the fallback). Stays #!/usr/bin/env bash, set -euo pipefail, shellcheck -S style clean, executable. The script still never judges review pass/fail or risk (Faber's call) — it enforces only the mechanical gate.

Self-modification note

merge-pr.sh is run from disk by Faber's in-session auto-merge flow — this change takes live effect on merge; no /faber re-sync needed (that regeneration is only required when templates/faber-command.md changes, which it does not here).

How verified locally

• bash -n scripts/merge-pr.sh → clean.
• shellcheck -S style scripts/merge-pr.sh (and all scripts/*.sh) → clean.
• Structure check (ci/required-files.txt manifest, run under bash) → structure ok; merge-pr.sh is +x. No manifest changes (only edits to already-listed files).
• jq logic for required-context extraction and bucket lookup validated against representative payloads: merges+dedups contexts + checks[].context; empty for no-required and 404 cases; a required context absent from reported checks resolves to missing (blocks); an optional pending check is ignored when not in the required set.

New refuse/branch paths (reasoned — no unrelated PR was merged)

• No merge method allowed → refuse: "repo … allows no merge method (squash/merge/rebase all disabled)".
• reviewDecision=REVIEW_REQUIRED → refuse: comments-only reviewer never approves; hand to human gate.
• Required check failing/absent → refuse, naming each offending required context (optional checks listed as ignored).
• No required checks + optional/all checks not green → legacy refuse path, naming checks and noting "no required checks defined on base".

Out of scope

Deferred should-fix items and the unattended/cross-repo auto-merge (that's #53).

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 462acfe
Reviewed-base: 0983bfa

Posted verbatim by codex-review.sh (codex exec review --base refs/codex-review/59-8723/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The new required-checks gate can mis-detect protected branches whose names require URL encoding and incorrectly apply the legacy all-checks gate. This is a functional regression for common release branch naming patterns.

Review comment:

• [P2] Encode base branch before branch-protection API call — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.DPQAjs1pGW/scripts/merge-pr.sh:238-238
  When the target branch name contains characters that need URL escaping, especially common names like release/1.0, interpolating $base_ref directly into this REST path sends those slashes as path separators. The required-status-checks lookup then 404s and the script falls back to gating on all checks, so a protected release PR with passing required checks but a pending/failing optional check is incorrectly refused. URL-encode the branch path component or use an API path/helper that does so before calling gh api.

[yihanzhu]

Round-0 fix (Coder) — [P2] URL-encode base branch in protection lookup

Head: 3c625bf3a2286c65cd27ce7fe3e7e7a129109ffd

The branch-protection lookup interpolated $base_ref raw into the REST path
(repos/{owner}/{repo}/branches/${base_ref}/protection/required_status_checks).
A slashed base name (release/1.0, feature/x) sent its slashes as path
separators → 404 → 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 (the exact regression the required-vs-optional change
was meant to prevent).

Fix: URL-encode the branch path component with jq's @uri filter before the
gh api call (/ → %2F):

base_ref_enc="$(jq -rn --arg b "$base_ref" '$b | @uri')"

and use "$base_ref_enc" in the path. The 404/no-protection fallback and the
required-context gating are otherwise unchanged. (The diagnostic message still
prints the human-readable $base_ref name.) Added a comment explaining why.

Everything else from round-0 is preserved: merge-method detection,
required-vs-optional gate + no-protection fallback, reviewDecision refuse,
head/base SHA-pin guards, --match-head-commit, --repo-scoping, unset GH_REPO,
≥1-real-CI-pass; set -euo pipefail, executable.

Local checks green: shellcheck -S style scripts/merge-pr.sh, bash -n scripts/merge-pr.sh,
and the CI structure check (run under bash). Label bumped round-0 → round-1.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 3c625bf
Reviewed-base: 0983bfa

Posted verbatim by codex-review.sh (codex exec review --base refs/codex-review/59-9809/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The patch weakens the documented base-race guard by adding a network read after the final base verification, and it silently downgrades required-check discovery failures into the legacy fallback. These are correctness issues in the merge safety harness.

Full review comments:

• [P2] Move merge-method lookup before the final base check — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.DXdvHTJn5G/scripts/merge-pr.sh:308-308
  When the base branch is not protected with a server-side up-to-date/merge-queue gate, the script's only defense against merging onto an unreviewed base is the baseRefOid read immediately before gh pr merge. This new gh repo view call now happens after that read, so if another PR lands during this extra API round trip the script can still merge into a base Codex did not review. Fetch the repo merge-method flags before the final base check, or re-check the base after this call.

• [P2] Fail on required-check discovery errors — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.DXdvHTJn5G/scripts/merge-pr.sh:243-247
  When this protection endpoint is inaccessible for reasons other than “no required checks” (for example a write collaborator on a private repo without branch-protection read permission, a 403, rate limit, or transient 5xx), the script silently clears required_json and takes the legacy all-checks path. That misclassifies a protected branch as having no required checks, so pending/failing optional checks block the exact scenario this change is meant to allow and the diagnostic says no required checks are defined. Treat only a confirmed no-required-checks response as fallback, otherwise fail closed or use gh pr checks --required.

[yihanzhu]

Round-1 fixes pushed (head 64a73a3)

Both [P2]s in scripts/merge-pr.sh addressed:

Fix 1 — base-race read kept last. Moved the merge-method detection (gh repo view --json squashMergeAllowed,…) to before the final base-race re-check, so the baseRefOid read is once again the last API call before gh pr merge. The round-0 ordering put that extra network round-trip after the base read, reopening the window where another PR could land on the base and we'd merge onto a base Codex never reviewed. All other network reads (CI checks, review-decision, head re-check, merge-method) now happen earlier on purpose.

Fix 2 — fail CLOSED on indeterminate required-checks discovery. The …/protection/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 (no branch-protection read access), 429 rate-limit, 5xx, or a network error — now refuses with an actionable message naming the HTTP cause (error: could not determine required status checks for '<base>' (HTTP <code>); refusing to merge — re-run, or grant the gh operator branch-protection read access) and exits non-zero, instead of misclassifying a protected base as 'no required checks' (which would let an optional pending/failing check block the merge and make the diagnostic lie). Implemented by capturing gh api's stderr via mktemp and detecting HTTP 404. The genuine-404 legacy fallback (≥1 pass, none failing) and the round-1 jq @uri branch URL-encoding are preserved.

Header + inline comments updated to reflect the reordering and the fail-closed semantics. Everything else from prior rounds is intact (SHA-pin head/base guards, --match-head-commit, --repo-scoping, unset GH_REPO, reviewDecision refuse, required-vs-optional gating, ≥1-real-CI-pass).

Local checks green: shellcheck -S style scripts/merge-pr.sh, bash -n scripts/merge-pr.sh, and the CI structure check (under bash).

Round bumped round-1 → round-2. Ready for re-review.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 64a73a3
Reviewed-base: 0983bfa

Posted verbatim by codex-review.sh (codex exec review --base refs/codex-review/59-11636/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The new required-check discovery depends on a branch-protection REST endpoint that many legitimate merge operators cannot read, causing valid green PRs to be refused. This breaks the merge helper in a common protected-repo permission setup.

Review comment:

• [P2] Avoid admin-only required-check lookup — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.JbqDFBW95Q/scripts/merge-pr.sh:262-262
  On protected repos where the gh operator can merge PRs but does not have Administration/branch-protection read permission, this new gh api .../protection/required_status_checks call returns a permission error and the fail-closed path aborts before merging even when the PR's required checks are green. That makes merge-pr.sh unusable for common non-admin maintainer setups not listed as a prerequisite; prefer the PR status-check rollup/gh pr checks --required, or explicitly require and validate branch-protection read access.

[yihanzhu]

Round-3 fix — [P2]: required-check discovery no longer needs admin permission

Head: 3dca017

Problem

Required-check discovery used gh api repos/{owner}/{repo}/branches/{branch}/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 round-2's fail-closed path then refused even a green PR — breaking a common protected-repo maintainer setup.

Fix — discover required checks from the PR's status-check rollup

Replaced the protection-REST discovery with gh pr checks <pr> --required, which derives "required" from the PR rollup's isRequired flag — readable by anyone who can view the PR (no admin permission needed).

Discovery logic (decide from output CONTENT, not exit code — gh pr checks exits non-zero merely for pending checks):

1. req_out is a non-empty JSON array → required checks present → gate on exactly those: refuse unless every entry's bucket is pass (names the non-pass ones); optional/non-required checks are ignored.
2. Else if output/stderr contains no required checks (no JSON array) → legacy fallback on the FULL set: gh pr checks --json name,bucket → require ≥1 pass and zero failing/non-skipped; refuse on zero checks. (Unchanged from the existing fallback.)
3. Else (neither an array nor the benign message — genuine auth/network error) → fail closed: refuse with the captured gh error.

Each path prints an accurate diagnostic naming which branch ran (gated on N required checks / legacy fallback because no required checks / refused because discovery failed).

Removed (now obsolete):

• the round-1 jq @uri branch URL-encoding (no branch name goes in a URL anymore);
• the round-2 gh api .../protection/required_status_checks call and its 404-vs-error fail-closed distinction (incl. the mktemp errfile dance for that call — the rollup path keeps its own errfile).

Net: −113 / +94 in merge-pr.sh — a simplification.

Preserved (no regression to round-2's other fix)

• Merge-method detection stays BEFORE the final base read; the baseRefOid read remains the LAST API call before gh pr merge. All gh pr checks reads (a read) happen earlier in the CI-gate block, before merge-method detection.
• Kept: reviewDecision=REVIEW_REQUIRED refuse, merge-method detection + --match-head-commit, head/base SHA-pin guards, --repo-scoping, unset GH_REPO. Still set -euo pipefail, shellcheck-clean (-S style), executable.
• templates/repo-setup.md branch-protection section now states discovery uses the PR rollup (no branch-protection read access needed; non-admin maintainers supported).

Testing

Ran the new discovery block against PR #59 itself (gh 2.92.0, no required checks configured):

• gh pr checks 59 --required --json name,state,bucket → rc≠0, no JSON, stderr no required checks reported on the '<branch>' branch → classified as legacy fallback (path 2).
• Full set gh pr checks 59 --json name,bucket → [{"bucket":"pass","name":"ci"}] → 1 passing, none failing/pending → gate passes, does NOT fail closed. ✅

Reasoning for the other two paths:

• Required-present (path 1): when the rollup reports required checks, req_out is a non-empty JSON array; jq -e 'type=="array" and length>0' selects this branch, and the gate refuses unless every entry is bucket=="pass", ignoring optional checks. (The previously-failing non-admin scenario now lands here or in path 2 — never errors on a permission it lacks.)
• Genuine error (path 3): an auth/network failure yields neither a JSON array nor the benign message, so it falls through to fail-closed and refuses with the captured gh error — preserving round-2's "never merge with unverified checks" guarantee, now reserved for actual errors rather than a routine missing-permission case.

Local checks (green, under bash)

• shellcheck -S style scripts/merge-pr.sh → clean
• bash -n scripts/merge-pr.sh → ok
• structure check (ci/required-files.txt manifest, bash) → structure ok

Round label bumped round-2 → round-3 (the cap). No merge/approve performed — stopping for review.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 3dca017
Reviewed-base: 0983bfa

Posted verbatim by codex-review.sh (codex exec review --base refs/codex-review/59-12819/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

No discrete, actionable correctness issues were identified in the reviewed diff. The changes preserve the existing safety gates while adding required-check filtering, review-required refusal, and merge-method selection.