From fc7da772825ad0498b55ae888d4c3a83475e6fe3 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 8 Jun 2026 07:59:12 +0300 Subject: [PATCH 1/3] ci: gatekeeper App auto-approver bridge scaffold (MCP-1249) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bridge that turns a Paperclip Codex review verdict of ACCEPT into a real GitHub approving review posted by the "MCPProxy Gatekeeper" GitHub App, so the required-1-approving-review branch protection is satisfied without an admin override and without the author approving their own PR (author!=approver). Pairs with scripts/arm-auto-merge.sh for full Model B. - Reads verdict of record from the Paperclip review thread (bots don't post to GitHub); only ACCEPT approves, request_changes/unknown are no-ops. - Mints a GitHub App installation token (RS256 JWT via openssl, no extra deps). - Inert until configured (GATEKEEPER_APP_ID / _INSTALLATION_ID / _PRIVATE_KEY, e.g. ~/.mcpproxy-gatekeeper/env) — exits 2 with setup guidance. - --dry-run + --verdict overrides for testing. Validated against live data: reads accept for PR #622, fails safe unconfigured, no-ops on request_changes. App registration + cred wiring is the remaining (owner) step. Related MCP-1249 --- scripts/gatekeeper-approve.sh | 168 ++++++++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100755 scripts/gatekeeper-approve.sh diff --git a/scripts/gatekeeper-approve.sh b/scripts/gatekeeper-approve.sh new file mode 100755 index 000000000..fa98444a7 --- /dev/null +++ b/scripts/gatekeeper-approve.sh @@ -0,0 +1,168 @@ +#!/usr/bin/env bash +# gatekeeper-approve.sh — "Gatekeeper" GitHub App auto-approver bridge (MCP-1249). +# +# Purpose: turn a Paperclip Codex review verdict of ACCEPT into a real GitHub +# *approving review* posted by a branded GitHub App identity, so the repo's +# "1 approving review by a reviewer with write access" branch-protection rule is +# satisfied WITHOUT an admin override and WITHOUT the PR author approving their +# own PR (author != approver). Once the App approval lands, GitHub auto-merge +# (squash) completes the merge with zero admin — full "Model B". +# +# This is the missing piece of MCP-1249. It pairs with arm-auto-merge.sh. +# +# Verdict source of record = the Paperclip review comment (the review bots do NOT +# post to GitHub). This script reads that verdict and, only on ACCEPT, posts the +# GitHub approval as the App. +# +# ───────────────────────────────────────────────────────────────────────────── +# Configuration (env, or sourced from a gitignored file — NEVER commit secrets): +# GATEKEEPER_APP_ID GitHub App ID (integer) +# GATEKEEPER_INSTALLATION_ID Installation ID for smart-mcp-proxy/mcpproxy-go +# GATEKEEPER_PRIVATE_KEY Path to the App private key .pem +# GATEKEEPER_REPO (optional) owner/repo, default smart-mcp-proxy/mcpproxy-go +# PAPERCLIP_API_URL (optional) default http://localhost:3100 +# PAPERCLIP_COMPANY_ID (optional) default 16edd8ed-8691-4a89-aa30-74ab6b931663 +# CODEX_REVIEWER_AGENT_ID (optional) default 5b94562c-524f-4c29-bc24-3524c1acd8e9 +# A convenient place: ~/.mcpproxy-gatekeeper/env (chmod 600, gitignored). +# +# Usage: +# gatekeeper-approve.sh --pr [--verdict accept|request_changes] [--dry-run] +# +# --pr (required) PR number to act on. +# --verdict override the Paperclip verdict lookup (testing). +# --dry-run do everything except POST the review (and print the plan). +# +# Exit codes: 0 ok/approved/dry-run; 2 not configured; 3 verdict not accept +# (no-op); 4 author==approver guard; 5 GitHub/API error. +# ───────────────────────────────────────────────────────────────────────────── +set -euo pipefail + +REPO="${GATEKEEPER_REPO:-smart-mcp-proxy/mcpproxy-go}" +PAPERCLIP_API_URL="${PAPERCLIP_API_URL:-http://localhost:3100}" +PAPERCLIP_COMPANY_ID="${PAPERCLIP_COMPANY_ID:-16edd8ed-8691-4a89-aa30-74ab6b931663}" +CODEX_REVIEWER_AGENT_ID="${CODEX_REVIEWER_AGENT_ID:-5b94562c-524f-4c29-bc24-3524c1acd8e9}" + +# Optional config file +[[ -f "${HOME}/.mcpproxy-gatekeeper/env" ]] && source "${HOME}/.mcpproxy-gatekeeper/env" + +PR=""; VERDICT_OVERRIDE=""; DRY_RUN=0 +while [[ $# -gt 0 ]]; do + case "$1" in + --pr) PR="$2"; shift 2;; + --verdict) VERDICT_OVERRIDE="$2"; shift 2;; + --dry-run) DRY_RUN=1; shift;; + -h|--help) grep '^#' "$0" | sed 's/^# \{0,1\}//'; exit 0;; + *) echo "unknown arg: $1" >&2; exit 1;; + esac +done +[[ -z "$PR" ]] && { echo "ERROR: --pr required" >&2; exit 1; } + +log() { echo "[gatekeeper] $*" >&2; } + +# ── 1. Resolve the Codex review verdict for this PR from Paperclip ─────────── +resolve_verdict() { + if [[ -n "$VERDICT_OVERRIDE" ]]; then echo "$VERDICT_OVERRIDE"; return; fi + # Reads are fine unauthenticated against the local instance. + curl -fsS -m 15 "${PAPERCLIP_API_URL}/api/companies/${PAPERCLIP_COMPANY_ID}/issues?q=Review%20PR%20%23${PR}" 2>/dev/null \ + | PR="$PR" CODEX="$CODEX_REVIEWER_AGENT_ID" BASE="$PAPERCLIP_API_URL" python3 -c ' +import sys, json, os, urllib.request +pr, codex, base = os.environ["PR"], os.environ["CODEX"], os.environ["BASE"] +iss = json.load(sys.stdin) +iss = iss if isinstance(iss, list) else iss.get("issues", iss.get("data", [])) +# Codex review tasks for this PR (any title that references "PR #"), +# assigned to the Codex reviewer, newest first (round-2 supersedes round-1). +needle = "PR #%s" % pr +revs = [i for i in iss + if needle in (i.get("title") or "") and i.get("assigneeAgentId") == codex] +revs.sort(key=lambda x: x.get("createdAt", ""), reverse=True) +verdict = "unknown" +for i in revs: + url = "%s/api/issues/%s/comments" % (base, i.get("id")) + try: + c = json.load(urllib.request.urlopen(url, timeout=15)) + except Exception: + continue + c = c if isinstance(c, list) else c.get("comments", c.get("data", [])) + for cm in reversed(c): + b = (cm.get("body") or "").lower() + if "verdict:" not in b: + continue + tail = b.split("verdict:", 1)[1][:40] + if "accept" in tail: + verdict = "accept"; break + if "request_changes" in tail or "request changes" in tail: + verdict = "request_changes"; break + if verdict != "unknown": + break +print(verdict) +' +} + +# ── 2. Mint a GitHub App installation access token (RS256 JWT via openssl) ──── +mint_installation_token() { + local app_id="$1" install_id="$2" pem="$3" + local now exp header payload b64 signing sig jwt + now=$(date +%s); exp=$((now + 540)) # 9-min window (max 10) + b64() { openssl base64 -A | tr '+/' '-_' | tr -d '='; } + header=$(printf '{"alg":"RS256","typ":"JWT"}' | b64) + payload=$(printf '{"iat":%d,"exp":%d,"iss":"%s"}' "$((now-60))" "$exp" "$app_id" | b64) + signing="${header}.${payload}" + sig=$(printf '%s' "$signing" | openssl dgst -sha256 -sign "$pem" -binary | b64) + jwt="${signing}.${sig}" + curl -fsS -m 20 -X POST \ + -H "Authorization: Bearer ${jwt}" \ + -H "Accept: application/vnd.github+json" \ + "https://api.github.com/app/installations/${install_id}/access_tokens" \ + | python3 -c 'import sys,json; print(json.load(sys.stdin)["token"])' +} + +# ── main ──────────────────────────────────────────────────────────────────── +VERDICT="$(resolve_verdict || echo unknown)" +log "PR #${PR} Codex verdict = ${VERDICT}" + +if [[ "$VERDICT" != "accept" ]]; then + log "verdict is not 'accept' — NOT approving (no-op). request_changes/unknown must not auto-approve." + exit 3 +fi + +# Author != approver guard. The App identity is inherently != the PR author, +# but verify the PR author is not somehow the bot (defense in depth). +AUTHOR="$(gh pr view "$PR" --repo "$REPO" --json author -q .author.login 2>/dev/null || echo '?')" +log "PR #${PR} author = ${AUTHOR} (App approves as a distinct identity)" + +if [[ -z "${GATEKEEPER_APP_ID:-}" || -z "${GATEKEEPER_INSTALLATION_ID:-}" || -z "${GATEKEEPER_PRIVATE_KEY:-}" ]]; then + log "NOT CONFIGURED: set GATEKEEPER_APP_ID, GATEKEEPER_INSTALLATION_ID, GATEKEEPER_PRIVATE_KEY" + log "(register the 'MCPProxy Gatekeeper' App, install on ${REPO}, drop creds in ~/.mcpproxy-gatekeeper/env)" + exit 2 +fi +[[ -f "$GATEKEEPER_PRIVATE_KEY" ]] || { log "private key not found: $GATEKEEPER_PRIVATE_KEY"; exit 2; } + +BODY="✅ **Gatekeeper approval** — Codex review verdict: ACCEPT. + +This approval is posted automatically by the MCPProxy Gatekeeper App on behalf of the Codex reviewer (verdict of record lives in the Paperclip review thread). Author≠approver satisfied; QA + CI gates enforced separately. + +_Auto-approved per Model B (MCP-1249)._" + +if [[ "$DRY_RUN" == "1" ]]; then + log "DRY-RUN: would mint installation token (app=${GATEKEEPER_APP_ID} install=${GATEKEEPER_INSTALLATION_ID}) and POST APPROVE review on ${REPO}#${PR}." + exit 0 +fi + +log "minting installation token…" +TOKEN="$(mint_installation_token "$GATEKEEPER_APP_ID" "$GATEKEEPER_INSTALLATION_ID" "$GATEKEEPER_PRIVATE_KEY")" \ + || { log "failed to mint installation token"; exit 5; } + +log "posting APPROVE review on ${REPO}#${PR}…" +HTTP=$(curl -s -o /tmp/gatekeeper-resp.json -w '%{http_code}' -m 20 -X POST \ + -H "Authorization: token ${TOKEN}" \ + -H "Accept: application/vnd.github+json" \ + "https://api.github.com/repos/${REPO}/pulls/${PR}/reviews" \ + --data "$(python3 -c 'import json,sys; print(json.dumps({"event":"APPROVE","body":sys.argv[1]}))' "$BODY")") + +if [[ "$HTTP" == "200" ]]; then + log "✅ approved ${REPO}#${PR} as Gatekeeper." + exit 0 +else + log "❌ GitHub review POST failed (HTTP ${HTTP}):"; cat /tmp/gatekeeper-resp.json >&2; echo >&2 + exit 5 +fi From 8f03a7b963deff6d515e2b49feea697267370bac Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 8 Jun 2026 18:31:39 +0300 Subject: [PATCH 2/3] =?UTF-8?q?ci:=20gatekeeper=20auto-trigger=20=E2=80=94?= =?UTF-8?q?=20sweep=20+=20launchd=20timer,=20hardened=20approve=20(MCP-124?= =?UTF-8?q?9)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hands-off half of Model B: a launchd timer runs gatekeeper-sweep.sh every 5m, which invokes gatekeeper-approve.sh for each open PR. Codex ACCEPT -> App approval -> GitHub auto-merge, no human, no admin. - gatekeeper-approve.sh hardened to be safe under repeated/unattended runs: - no-op if PR is closed/merged - stale-verdict guard: only approve the exact SHA Codex reviewed (head moved past it -> skip, re-review needed) [exit 6] - idempotency: skip if the Gatekeeper already approved the current head - resolver now also extracts the reviewer-pinned SHA - gatekeeper-sweep.sh: bash-3.2 safe (macOS /bin/bash, no mapfile); skips quietly when unconfigured or Paperclip unreachable; timestamped logging. - app.mcpproxy.gatekeeper.sweep.plist: launchd template (StartInterval 300, RunAtLoad, background/low-IO) + install/enable/disable instructions. Validated: dry sweep over 14 open PRs no-ops correctly; idempotent no-op on the already-merged #622; timer loaded and first run logged. Related MCP-1249 --- scripts/app.mcpproxy.gatekeeper.sweep.plist | 32 ++++++++++ scripts/gatekeeper-approve.sh | 71 ++++++++++++++++----- scripts/gatekeeper-sweep.sh | 62 ++++++++++++++++++ 3 files changed, 149 insertions(+), 16 deletions(-) create mode 100644 scripts/app.mcpproxy.gatekeeper.sweep.plist create mode 100755 scripts/gatekeeper-sweep.sh diff --git a/scripts/app.mcpproxy.gatekeeper.sweep.plist b/scripts/app.mcpproxy.gatekeeper.sweep.plist new file mode 100644 index 000000000..cae3344c7 --- /dev/null +++ b/scripts/app.mcpproxy.gatekeeper.sweep.plist @@ -0,0 +1,32 @@ + + + + + Label app.mcpproxy.gatekeeper.sweep + ProgramArguments + + /bin/bash + __SWEEP__ + + StartInterval 300 + RunAtLoad + StandardOutPath __LOG__ + StandardErrorPath __LOG__ + EnvironmentVariables + + PATH/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin + + ProcessType Background + LowPriorityIO + + diff --git a/scripts/gatekeeper-approve.sh b/scripts/gatekeeper-approve.sh index fa98444a7..7a020fd36 100755 --- a/scripts/gatekeeper-approve.sh +++ b/scripts/gatekeeper-approve.sh @@ -32,8 +32,12 @@ # --verdict override the Paperclip verdict lookup (testing). # --dry-run do everything except POST the review (and print the plan). # -# Exit codes: 0 ok/approved/dry-run; 2 not configured; 3 verdict not accept -# (no-op); 4 author==approver guard; 5 GitHub/API error. +# Safe to run repeatedly (idempotent): no-ops if the PR is closed/merged, if the +# verdict isn't ACCEPT, if the Gatekeeper already approved the current head, or +# if Codex's reviewed SHA != the current head (stale → needs re-review). +# +# Exit codes: 0 ok/approved/dry-run/no-op; 2 not configured; 3 verdict not accept; +# 5 GitHub/API error; 6 stale verdict (head moved past reviewed SHA). # ───────────────────────────────────────────────────────────────────────────── set -euo pipefail @@ -59,23 +63,24 @@ done log() { echo "[gatekeeper] $*" >&2; } -# ── 1. Resolve the Codex review verdict for this PR from Paperclip ─────────── +# ── 1. Resolve the Codex review verdict (+reviewed SHA) from Paperclip ─────── +# Emits " ". resolve_verdict() { - if [[ -n "$VERDICT_OVERRIDE" ]]; then echo "$VERDICT_OVERRIDE"; return; fi + if [[ -n "$VERDICT_OVERRIDE" ]]; then echo "$VERDICT_OVERRIDE "; return; fi # Reads are fine unauthenticated against the local instance. curl -fsS -m 15 "${PAPERCLIP_API_URL}/api/companies/${PAPERCLIP_COMPANY_ID}/issues?q=Review%20PR%20%23${PR}" 2>/dev/null \ | PR="$PR" CODEX="$CODEX_REVIEWER_AGENT_ID" BASE="$PAPERCLIP_API_URL" python3 -c ' -import sys, json, os, urllib.request +import sys, json, os, re, urllib.request pr, codex, base = os.environ["PR"], os.environ["CODEX"], os.environ["BASE"] iss = json.load(sys.stdin) iss = iss if isinstance(iss, list) else iss.get("issues", iss.get("data", [])) -# Codex review tasks for this PR (any title that references "PR #"), -# assigned to the Codex reviewer, newest first (round-2 supersedes round-1). +# Codex review tasks for this PR (title references "PR #"), assigned to the +# Codex reviewer, newest first (round-2 supersedes round-1). needle = "PR #%s" % pr revs = [i for i in iss if needle in (i.get("title") or "") and i.get("assigneeAgentId") == codex] revs.sort(key=lambda x: x.get("createdAt", ""), reverse=True) -verdict = "unknown" +verdict, sha = "unknown", "" for i in revs: url = "%s/api/issues/%s/comments" % (base, i.get("id")) try: @@ -84,17 +89,20 @@ for i in revs: continue c = c if isinstance(c, list) else c.get("comments", c.get("data", [])) for cm in reversed(c): - b = (cm.get("body") or "").lower() + body = cm.get("body") or "" + b = body.lower() if "verdict:" not in b: continue tail = b.split("verdict:", 1)[1][:40] + shas = re.findall(r"\b[0-9a-f]{40}\b", body) # SHA the reviewer pinned + sha = shas[-1] if shas else "" if "accept" in tail: verdict = "accept"; break if "request_changes" in tail or "request changes" in tail: verdict = "request_changes"; break if verdict != "unknown": break -print(verdict) +print(verdict, sha) ' } @@ -117,18 +125,49 @@ mint_installation_token() { } # ── main ──────────────────────────────────────────────────────────────────── -VERDICT="$(resolve_verdict || echo unknown)" -log "PR #${PR} Codex verdict = ${VERDICT}" +read -r VERDICT REVIEWED_SHA <<<"$(resolve_verdict || echo 'unknown ')" +log "PR #${PR} Codex verdict = ${VERDICT}${REVIEWED_SHA:+ (reviewed ${REVIEWED_SHA:0:9})}" if [[ "$VERDICT" != "accept" ]]; then log "verdict is not 'accept' — NOT approving (no-op). request_changes/unknown must not auto-approve." exit 3 fi -# Author != approver guard. The App identity is inherently != the PR author, -# but verify the PR author is not somehow the bot (defense in depth). -AUTHOR="$(gh pr view "$PR" --repo "$REPO" --json author -q .author.login 2>/dev/null || echo '?')" -log "PR #${PR} author = ${AUTHOR} (App approves as a distinct identity)" +# Current PR head + author (one API read). +read -r HEAD AUTHOR PR_STATE <<<"$(gh pr view "$PR" --repo "$REPO" --json headRefOid,author,state \ + -q '"\(.headRefOid) \(.author.login) \(.state)"' 2>/dev/null || echo '? ? ?')" +log "PR #${PR} state=${PR_STATE} head=${HEAD:0:9} author=${AUTHOR} (App approves as a distinct identity)" + +# Don't act on already-closed/merged PRs. +if [[ "$PR_STATE" != "OPEN" ]]; then + log "PR is ${PR_STATE} — nothing to do." + exit 0 +fi + +# Stale-verdict guard: only approve the exact SHA Codex reviewed. If the head +# moved past the reviewed commit, the verdict is stale → needs re-review. +if [[ -n "$REVIEWED_SHA" && -n "$HEAD" && "$REVIEWED_SHA" != "$HEAD" ]]; then + log "STALE: Codex reviewed ${REVIEWED_SHA:0:9} but head is ${HEAD:0:9} — NOT approving (re-review needed)." + exit 6 +fi + +# Idempotency: skip if the Gatekeeper already has an APPROVED review at this head. +ALREADY="$(gh api "repos/${REPO}/pulls/${PR}/reviews" 2>/dev/null \ + | HEAD="$HEAD" python3 -c ' +import sys, json, os +head = os.environ.get("HEAD","") +try: revs = json.load(sys.stdin) +except Exception: revs = [] +for r in revs: + u = (r.get("user") or {}) + if u.get("type") == "Bot" and "gatekeeper" in (u.get("login","").lower()) \ + and r.get("state") == "APPROVED" and (not head or r.get("commit_id") == head): + print("yes"); break +' 2>/dev/null)" +if [[ "$ALREADY" == "yes" ]]; then + log "already approved by Gatekeeper at head ${HEAD:0:9} — no-op (idempotent)." + exit 0 +fi if [[ -z "${GATEKEEPER_APP_ID:-}" || -z "${GATEKEEPER_INSTALLATION_ID:-}" || -z "${GATEKEEPER_PRIVATE_KEY:-}" ]]; then log "NOT CONFIGURED: set GATEKEEPER_APP_ID, GATEKEEPER_INSTALLATION_ID, GATEKEEPER_PRIVATE_KEY" diff --git a/scripts/gatekeeper-sweep.sh b/scripts/gatekeeper-sweep.sh new file mode 100755 index 000000000..3d6e6db2a --- /dev/null +++ b/scripts/gatekeeper-sweep.sh @@ -0,0 +1,62 @@ +#!/usr/bin/env bash +# gatekeeper-sweep.sh — auto-trigger for the Gatekeeper App (MCP-1249). +# +# Sweeps every OPEN PR in the repo and invokes gatekeeper-approve.sh for each. +# That call is idempotent and self-guarding: it approves a PR only when the +# Codex review verdict is ACCEPT for the *current* head and the Gatekeeper hasn't +# already approved that head; everything else is a no-op. So this script is safe +# to run unattended on a timer (launchd/cron) — it's the hands-off half of +# "full Model B": Codex ACCEPT → App approval → GitHub auto-merge, no admin. +# +# Reads creds from ~/.mcpproxy-gatekeeper/env (via gatekeeper-approve.sh). +# Designed for launchd: sets a sane PATH and logs each sweep with a timestamp. +# +# Usage: gatekeeper-sweep.sh [--dry-run] +# Env: GATEKEEPER_REPO (default smart-mcp-proxy/mcpproxy-go) +set -euo pipefail + +# launchd starts with a minimal PATH — make sure our tools resolve. +export PATH="/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:${PATH:-}" + +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO="${GATEKEEPER_REPO:-smart-mcp-proxy/mcpproxy-go}" +DRY=""; [[ "${1:-}" == "--dry-run" ]] && DRY="--dry-run" + +ts() { date '+%Y-%m-%dT%H:%M:%S%z'; } +log() { echo "$(ts) [sweep] $*"; } + +# Gatekeeper not configured → exit quietly (don't spam logs every interval). +[[ -f "${HOME}/.mcpproxy-gatekeeper/env" ]] || { log "not configured (~/.mcpproxy-gatekeeper/env missing) — skipping."; exit 0; } + +# Paperclip (verdict source) must be reachable; otherwise every approve call +# would no-op as 'unknown' anyway — skip the sweep to keep logs clean. +PAPERCLIP_API_URL="${PAPERCLIP_API_URL:-http://localhost:3100}" +if ! curl -fsS -m 5 "${PAPERCLIP_API_URL}/api/health" >/dev/null 2>&1; then + log "Paperclip not reachable at ${PAPERCLIP_API_URL} — skipping this sweep." + exit 0 +fi + +# bash 3.2 (macOS /bin/bash) safe — no mapfile. +PRS=() +while IFS= read -r n; do [[ -n "$n" ]] && PRS+=("$n"); done \ + < <(gh pr list --repo "$REPO" --state open --json number -q '.[].number' 2>/dev/null || true) +log "open PRs: ${#PRS[@]}${PRS:+ (${PRS[*]})}" + +approved=0 +[[ ${#PRS[@]} -eq 0 ]] && { log "no open PRs — done."; exit 0; } +for pr in "${PRS[@]}"; do + [[ -z "$pr" ]] && continue + set +e + out="$("$HERE/gatekeeper-approve.sh" --pr "$pr" $DRY 2>&1)"; rc=$? + set -e + case $rc in + 0) if echo "$out" | grep -q 'approved.*as Gatekeeper'; then + log "PR #$pr → APPROVED"; approved=$((approved+1)) + fi ;; # other rc=0 = idempotent/closed no-op, stay quiet + 3) : ;; # not accept — quiet + 6) log "PR #$pr → stale verdict (re-review needed)" ;; + 2) log "PR #$pr → gatekeeper not configured"; break ;; + *) log "PR #$pr → error (rc=$rc): $(echo "$out" | tail -1)" ;; + esac +done +log "sweep done — ${approved} newly approved." From b3691862658d537bd66d91953efc8ef373fe3298 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 15 Jun 2026 19:39:00 +0300 Subject: [PATCH 3/3] =?UTF-8?q?fix(gatekeeper):=20fail-closed=20SHA=20guar?= =?UTF-8?q?d=20=E2=80=94=20refuse=20approval=20without=20a=20matching=20re?= =?UTF-8?q?viewed=20SHA=20(MCP-1249)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex REQUEST_CHANGES (PR #687): the stale-SHA guard in gatekeeper-approve.sh ran only `if [[ -n "$REVIEWED_SHA" && ... ]]`, so a verdict that pinned NO SHA (REVIEWED_SHA empty) — or the manual --verdict override path, which always yielded an empty SHA — bypassed the check and auto-approved the PR's CURRENT head. An old ACCEPT could thus approve unreviewed code after a post-review force-push. Fix (fail-closed): - Make the SHA checks UNCONDITIONAL. No reviewed SHA resolvable -> REFUSE (exit 7, explicit message); reviewed SHA != head -> REFUSE (exit 6, stale); only reviewed SHA == head approves. - Add --reviewed-sha to pair with --verdict so the manual override is held to the same fail-closed requirement (no blind manual approvals either). - Document the invariant + new exit code 7 in the header. Adds scripts/gatekeeper-approve.test.sh: hermetic (stubs gh, --dry-run, no network/GitHub) regression covering all four Codex acceptance cases. Red on the old script (case 1 approved blind), green on the fix. Co-Authored-By: Paperclip --- scripts/gatekeeper-approve.sh | 48 ++++++++++++++++------- scripts/gatekeeper-approve.test.sh | 61 ++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 13 deletions(-) create mode 100755 scripts/gatekeeper-approve.test.sh diff --git a/scripts/gatekeeper-approve.sh b/scripts/gatekeeper-approve.sh index 7a020fd36..91e470676 100755 --- a/scripts/gatekeeper-approve.sh +++ b/scripts/gatekeeper-approve.sh @@ -26,18 +26,27 @@ # A convenient place: ~/.mcpproxy-gatekeeper/env (chmod 600, gitignored). # # Usage: -# gatekeeper-approve.sh --pr [--verdict accept|request_changes] [--dry-run] +# gatekeeper-approve.sh --pr [--verdict accept|request_changes] +# [--reviewed-sha ] [--dry-run] # -# --pr (required) PR number to act on. -# --verdict override the Paperclip verdict lookup (testing). -# --dry-run do everything except POST the review (and print the plan). +# --pr (required) PR number to act on. +# --verdict override the Paperclip verdict lookup (testing/manual). +# --reviewed-sha override the reviewed SHA (pairs with --verdict; required +# to approve via the manual override path — fail-closed). +# --dry-run do everything except POST the review (and print the plan). # # Safe to run repeatedly (idempotent): no-ops if the PR is closed/merged, if the -# verdict isn't ACCEPT, if the Gatekeeper already approved the current head, or -# if Codex's reviewed SHA != the current head (stale → needs re-review). +# verdict isn't ACCEPT, or if the Gatekeeper already approved the current head. +# +# FAIL-CLOSED SHA GUARD (MCP-1249 Codex REQUEST_CHANGES): the script approves the +# PR's CURRENT head ONLY when it can resolve a reviewed SHA that EQUALS that head. +# If no reviewed SHA can be resolved, it REFUSES (never approves blind) so a +# post-review force-push of unreviewed code cannot inherit an old ACCEPT. The +# manual --verdict accept override is held to the same requirement. # # Exit codes: 0 ok/approved/dry-run/no-op; 2 not configured; 3 verdict not accept; -# 5 GitHub/API error; 6 stale verdict (head moved past reviewed SHA). +# 5 GitHub/API error; 6 stale verdict (reviewed SHA != head); +# 7 no reviewed SHA resolvable (fail-closed refusal). # ───────────────────────────────────────────────────────────────────────────── set -euo pipefail @@ -49,11 +58,12 @@ CODEX_REVIEWER_AGENT_ID="${CODEX_REVIEWER_AGENT_ID:-5b94562c-524f-4c29-bc24-3524 # Optional config file [[ -f "${HOME}/.mcpproxy-gatekeeper/env" ]] && source "${HOME}/.mcpproxy-gatekeeper/env" -PR=""; VERDICT_OVERRIDE=""; DRY_RUN=0 +PR=""; VERDICT_OVERRIDE=""; REVIEWED_SHA_OVERRIDE=""; DRY_RUN=0 while [[ $# -gt 0 ]]; do case "$1" in --pr) PR="$2"; shift 2;; --verdict) VERDICT_OVERRIDE="$2"; shift 2;; + --reviewed-sha) REVIEWED_SHA_OVERRIDE="$2"; shift 2;; --dry-run) DRY_RUN=1; shift;; -h|--help) grep '^#' "$0" | sed 's/^# \{0,1\}//'; exit 0;; *) echo "unknown arg: $1" >&2; exit 1;; @@ -66,7 +76,7 @@ log() { echo "[gatekeeper] $*" >&2; } # ── 1. Resolve the Codex review verdict (+reviewed SHA) from Paperclip ─────── # Emits " ". resolve_verdict() { - if [[ -n "$VERDICT_OVERRIDE" ]]; then echo "$VERDICT_OVERRIDE "; return; fi + if [[ -n "$VERDICT_OVERRIDE" ]]; then echo "$VERDICT_OVERRIDE ${REVIEWED_SHA_OVERRIDE:-}"; return; fi # Reads are fine unauthenticated against the local instance. curl -fsS -m 15 "${PAPERCLIP_API_URL}/api/companies/${PAPERCLIP_COMPANY_ID}/issues?q=Review%20PR%20%23${PR}" 2>/dev/null \ | PR="$PR" CODEX="$CODEX_REVIEWER_AGENT_ID" BASE="$PAPERCLIP_API_URL" python3 -c ' @@ -144,10 +154,22 @@ if [[ "$PR_STATE" != "OPEN" ]]; then exit 0 fi -# Stale-verdict guard: only approve the exact SHA Codex reviewed. If the head -# moved past the reviewed commit, the verdict is stale → needs re-review. -if [[ -n "$REVIEWED_SHA" && -n "$HEAD" && "$REVIEWED_SHA" != "$HEAD" ]]; then - log "STALE: Codex reviewed ${REVIEWED_SHA:0:9} but head is ${HEAD:0:9} — NOT approving (re-review needed)." +# Fail-closed stale-verdict guard (MCP-1249 Codex REQUEST_CHANGES): we approve +# ONLY the exact SHA the reviewer reviewed. The checks below are UNCONDITIONAL — +# a missing or non-matching reviewed SHA must REFUSE, never approve blind. This +# closes the hole where an old ACCEPT (or a verdict that pinned no SHA) would +# auto-approve the current head after a post-review force-push of unreviewed code. +if [[ -z "$HEAD" ]]; then + log "REFUSING: could not resolve the PR head SHA — fail-closed (will not approve)." + exit 5 +fi +if [[ -z "$REVIEWED_SHA" ]]; then + log "REFUSING: no reviewed SHA could be resolved from the ACCEPT verdict — fail-closed (will not approve blind)." + log " The reviewer must pin the reviewed commit SHA in the verdict comment; for a manual override pass --reviewed-sha ." + exit 7 +fi +if [[ "$REVIEWED_SHA" != "$HEAD" ]]; then + log "STALE: reviewer reviewed ${REVIEWED_SHA:0:9} but head is ${HEAD:0:9} — NOT approving (re-review needed)." exit 6 fi diff --git a/scripts/gatekeeper-approve.test.sh b/scripts/gatekeeper-approve.test.sh new file mode 100755 index 000000000..756e3769c --- /dev/null +++ b/scripts/gatekeeper-approve.test.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# gatekeeper-approve.test.sh — fail-closed SHA-guard regression tests (MCP-1249). +# +# Proves the security invariant Codex flagged: gatekeeper-approve.sh must REFUSE +# to post an approving review unless it can resolve a reviewed SHA that equals +# the PR's current head. A missing reviewed SHA must fail CLOSED (never approve +# blind), so a post-review force-push of unreviewed code cannot inherit an old +# ACCEPT. The manual --verdict accept override is held to the same requirement. +# +# Hermetic: stubs `gh` on PATH and uses --verdict/--dry-run so no network, no +# Paperclip, and no real GitHub approval is ever posted. +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +APPROVE="$SCRIPT_DIR/gatekeeper-approve.sh" +TMP="$(mktemp -d)" +trap 'rm -rf "$TMP"' EXIT + +HEAD_SHA="aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" +OTHER_SHA="bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + +# Stub gh: PR head is OPEN at HEAD_SHA, no existing reviews. +cat > "$TMP/gh" < fail-closed refuse, must NOT approve. +out="$("$APPROVE" --pr 999 --verdict accept --dry-run 2>&1)"; rc=$? +check "no reviewed SHA -> refuse (fail-closed)" "7" "$rc" +if refused "$out"; then echo " msg: refusal text present"; else echo " FAIL: no refusal text"; fail=$((fail+1)); fi +if echo "$out" | grep -qi "DRY-RUN: would"; then echo " FAIL: reached approve with no SHA!"; fail=$((fail+1)); fi + +# 2. accept, reviewed SHA != head -> stale refuse. +out="$("$APPROVE" --pr 999 --verdict accept --reviewed-sha "$OTHER_SHA" --dry-run 2>&1)"; rc=$? +check "reviewed SHA != head -> stale refuse" "6" "$rc" +if echo "$out" | grep -qi "DRY-RUN: would"; then echo " FAIL: reached approve while stale!"; fail=$((fail+1)); fi + +# 3. accept, reviewed SHA == head -> reaches approve (dry-run, exit 0). +out="$("$APPROVE" --pr 999 --verdict accept --reviewed-sha "$HEAD_SHA" --dry-run 2>&1)"; rc=$? +check "reviewed SHA == head -> approves (dry-run)" "0" "$rc" +if echo "$out" | grep -qi "DRY-RUN: would"; then echo " reached approve step as expected"; else echo " FAIL: did not reach approve"; fail=$((fail+1)); fi + +echo "----" +echo "pass=$pass fail=$fail" +[[ "$fail" == "0" ]]