Skip to content

fix(pr-merge): refuse to merge a PR whose forge checks are red or unreadable - #2299

Open
Nikita-Guzenko wants to merge 1 commit into
kunchenguid:mainfrom
Nikita-Guzenko:fm/pr-merge-refuse-red-checks
Open

fix(pr-merge): refuse to merge a PR whose forge checks are red or unreadable#2299
Nikita-Guzenko wants to merge 1 commit into
kunchenguid:mainfrom
Nikita-Guzenko:fm/pr-merge-refuse-red-checks

Conversation

@Nikita-Guzenko

Copy link
Copy Markdown

Intent

Make bin/fm-pr-merge.sh refuse to merge a pull request whose forge reports a failing check, so that "never merge a red PR" is enforced by the one merge path instead of by session attention.

Why: on 2026-08-10 the merge-queue run for nguzen/aln pull request 182 failed at 17:54 UTC and the pull request was merged at 17:59 UTC by the same account; the defect in that commit then blocked every production deploy until 2026-08-12, and the product owner asked about missing changes twice. bin/fm-pr-merge.sh read no check state at all, and the no-mistakes CI monitor on this box cannot read check results either (its gh pr checks call fails), so two blind spots combined into a two-day outage.

Requirements the user stated, all deliberate:

  1. Read the forge check state before merging and REFUSE with an error exit code, listing exactly what is red.
  2. The merge queue must be covered: it re-runs the checks on the combined commit, so its verdict can differ from the branch verdict. A branch-only read was explicitly called insufficient.
  3. An explicit override flag for a deliberate red merge (for example an infrastructurally broken check). Without the flag, refuse.
  4. An unreadable check state is a REFUSAL, not permission. Forge silence must never read as green - that is the exact class of error that caused the incident.
  5. Do not break GitLab/Forgejo behavior already present in bin/fm-pr-lib.sh; where check state is unavailable, the refusal must be intelligible rather than cryptic.

Named trap, verified 2026-08-12: gh pr checks exits non-zero both when something failed and when something is still running, so classification by exit status is impossible - only output values can classify. That is why this change does not use gh pr checks at all.

Decisions made while implementing, so they are not mistakes:

  • New sourced library bin/fm-pr-checks-lib.sh owns the classification; bin/fm-pr-merge.sh only decides. It reads two sources: gh pr view --json number,baseRefName,statusCheckRollup for the PR head, and gh api "repos///actions/runs?event=merge_group&per_page=100" filtered to head_branch prefix gh-readonly-queue//pr-- for the merge queue. Both classified with jq by value.
  • Only the NEWEST queue attempt is judged, so a superseded red attempt cannot block a re-queued green pull request; within that attempt the newest run per workflow wins so a re-run replaces its own result.
  • Branch-side CANCELLED and STALE count as failures (the branch is not green and nothing re-runs them), while queue-side cancelled/stale count as inconclusive because the queue itself discards and re-runs those attempts as PRs ahead land.
  • Every read failure - missing gh or jq, a failed call, a bound hit, output that does not parse, a payload describing a different pull request, or a payload missing the fields that were requested - returns unreadable and refuses. jq and gh are hard requirements on this path, consistent with bin/fm-fleet-view.sh and bin/fm-bearings-snapshot.sh.
  • pending (checks still running) is permitted but printed as a visible note; only failing and unreadable refuse. The forge branch protection remains the backstop for pending.
  • The gate runs BEFORE bin/fm-pr-check.sh, so a refused merge records no pr= metadata and arms no merge poll, matching the script prior refusals.
  • --allow-failing-checks skips the read entirely (it must also override an unreadable verdict, which is the infrastructural-breakage case) and announces itself on stderr. Own flags are consumed before the -- separator; an unknown own flag is a usage error with exit 2 rather than a flag silently forwarded to gh-axi. This does mean extra gh-axi flags must now come after --, which is the documented usage and what every existing test already does.
  • GitLab is untouched: bin/fm-pr-merge.sh already refuses a GitLab merge request URL by provider before the gate can run, and the library header records that it is GitHub-only for that reason.
  • AGENTS.md gained one line recording that --allow-failing-checks IS a red merge and needs the same explicit captain instruction any red merge needs, so the flag cannot be treated as a routine convenience. docs/architecture.md and docs/scripts.md were updated to match.

Verification already performed: the library classifies the live incident correctly - nguzen/aln#182 as failing (branch check "e2e visual QA (Playwright)" plus merge-queue check CI) and #183 as green. tests/fm-pr-merge.test.sh gained seven cases (red branch, red merge queue with a green branch, superseded queue attempt, unreadable rollup read, unreadable queue read, payload for another pull request, override flag, unknown own flag) and passes 17/17; tests/fm-pr-check-security.test.sh passes 36/36 after its gh mock was taught the two new reads; bin/fm-lint.sh, bin/fm-doc-audience-check.sh and bin/fm-test-run.sh --check-coverage are clean. Both guards were mutation-checked: blinding the queue read fails only the queue case, and accepting an unreadable verdict fails only the unreadable case.

One factual correction to the original report, deliberately reflected in the code and the commit message: pull request 182 was red on BOTH sources, not only the queue - its own branch run failed the same visual-QA check at 17:56:50. The branch-green/queue-red split is therefore covered by a synthetic test fixture rather than by that live pull request, and both sources are read because either can be red alone.

What Changed

  • Added bin/fm-pr-checks-lib.sh, a sourced GitHub-only library that classifies a PR's forge check state from two independent sources — the PR head's statusCheckRollup (via gh pr view) and the newest merge-queue attempt's Actions runs (via gh api ...runs?event=merge_group) — using jq value inspection rather than gh pr checks exit codes, which cannot distinguish failing from still-running.
  • Gated bin/fm-pr-merge.sh on that verdict before it runs fm-pr-check.sh: a failing or unreadable state refuses the merge with a non-zero exit and prints exactly which checks are red, while pending is permitted with a visible note. Branch-side CANCELLED/STALE count as failures; queue-side cancelled/stale count as inconclusive since the queue re-runs them. Only the newest queue attempt (newest run per workflow) is judged so a superseded red attempt cannot block a re-queued green PR.
  • Added an --allow-failing-checks override (consumed as an own flag before --, announced on stderr) that skips the read entirely for deliberate red/infrastructural merges; an unknown own flag is now a usage error (exit 2) rather than being forwarded to gh-axi.
  • Expanded tests/fm-pr-merge.test.sh (17/17) and tests/fm-pr-check-security.test.sh with fixtures for red branch, branch-green/queue-red, superseded queue attempt, unreadable rollup/queue reads, foreign-PR payload, the override flag, and the unknown-flag case; updated AGENTS.md, docs/architecture.md, and docs/scripts.md to record that --allow-failing-checks is itself a red merge.

Risk Assessment

✅ Low: Well-bounded, additive change with a new sourced classifier and a pre-state merge gate; every required intent criterion is met, error paths fail closed to refusal, and it is covered by mutation-checked tests.

Testing

Ran the two changed suites and built an end-to-end demo. The primary target, tests/fm-pr-merge.test.sh, passes 17/17 and directly exercises each intent requirement including the merge-queue-red / branch-green split and the unreadable-is-refusal rule. tests/fm-pr-check-security.test.sh completed with exit 0 and no failures, confirming the new forge reads don't regress the existing recording/derivation and teardown-safety behavior. As the reviewer-visible artifact I drove the real fm-pr-merge.sh with a stubbed forge and captured the actual stderr for all six verdicts (red branch, branch-green/queue-red, unreadable, --allow-failing-checks, all-green, unknown flag), showing refusals exit 1 before arming the check poll while green/override proceed to the gh-axi merge call. This is a CLI/operator-facing change with no rendered UI surface, so the CLI transcript is the appropriate end-user evidence. Worktree left clean; evidence written only under /tmp.

Evidence: End-to-end merge-gate CLI transcript (6 verdicts against the real script)

=== 1. RED branch check === error: refusing to merge .../pull/182: the forge reports failing checks branch check: e2e visual QA (Playwright) (FAILURE) [exit 1] === 2. Branch GREEN but MERGE-QUEUE RED === error: refusing to merge ...: the forge reports failing checks merge-queue check: CI (FAILURE) (merge-queue attempt read: gh-readonly-queue/main/pr-182-abc) [exit 1] === 3. UNREADABLE (queue read fails) === error: refusing to merge ...: the forge's check state is unreadable (the merge-queue check runs of nguzen/aln could not be listed) [exit 1] === 4. --allow-failing-checks === warning: merging ... without reading the forge's check verdict (--allow-failing-checks) >> gh-axi pr merge 182 --repo nguzen/aln --squash (MERGE EXECUTED) [exit 0] === 5. All GREEN === >> gh-axi pr merge 182 --repo nguzen/aln --squash (MERGE EXECUTED) [exit 0] === 6. Unknown own flag === error: unknown merge flag --force (own flags: --allow-failing-checks; pass gh-axi flags after --) [exit 2]

=== 1. RED branch check (the incident: e2e visual QA failed) ===
error: refusing to merge https://github.com/nguzen/aln/pull/182: the forge reports failing checks
  branch check: e2e visual QA (Playwright) (FAILURE)
hint: land a green head, or pass --allow-failing-checks when the failure is infrastructural
  [exit 1]

=== 2. Branch GREEN but MERGE-QUEUE RED (the 2026-08-10 blind spot) ===
error: refusing to merge https://github.com/nguzen/aln/pull/182: the forge reports failing checks
  merge-queue check: CI (FAILURE)
  (merge-queue attempt read: gh-readonly-queue/main/pr-182-abc)
hint: land a green head, or pass --allow-failing-checks when the failure is infrastructural
  [exit 1]

=== 3. UNREADABLE (forge silence — queue read fails) — refuse, not pass ===
error: refusing to merge https://github.com/nguzen/aln/pull/182: the forge's check state is unreadable (the merge-queue check runs of nguzen/aln could not be listed)
hint: an unreadable verdict is never a pass; fix the read, or pass --allow-failing-checks deliberately
  [exit 1]

=== 4. --allow-failing-checks: deliberate red merge, announced on stderr ===
warning: merging https://github.com/nguzen/aln/pull/182 without reading the forge's check verdict (--allow-failing-checks)
●━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
●  WATCHER DOWN - SUPERVISION IS OFF
●  1 task(s) in flight, but no watcher has a fresh beacon (last beat: never, grace 300s).
●  Trust the emitted supervision protocol for this harness; do not use shell & for watcher repair.
●  This is a supervision warning only; the guarded operation WILL still run.
●  watcher supervision needs Stop-owned automatic recovery; inspect the hook registration and startup status before ending the turn.
●━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
armed: state/task-x1.check.sh
  >> gh-axi pr merge 182 --repo nguzen/aln --squash  (MERGE EXECUTED)
  [exit 0]

=== 5. All GREEN (branch + newest queue attempt) — merge proceeds ===
●━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
●  WATCHER DOWN - SUPERVISION IS OFF
●  1 task(s) in flight, but no watcher has a fresh beacon (last beat: never, grace 300s).
●  Trust the emitted supervision protocol for this harness; do not use shell & for watcher repair.
●  This is a supervision warning only; the guarded operation WILL still run.
●  watcher supervision needs Stop-owned automatic recovery; inspect the hook registration and startup status before ending the turn.
●━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
armed: state/task-x1.check.sh
  >> gh-axi pr merge 182 --repo nguzen/aln --squash  (MERGE EXECUTED)
  [exit 0]

=== 6. Unknown own flag — usage error, exit 2, never forwarded to gh-axi ===
error: unknown merge flag --force (own flags: --allow-failing-checks; pass gh-axi flags after --)
  [exit 2]
Evidence: Demo driver script (reusable)
#!/usr/bin/env bash
# End-to-end demonstration of the merge gate: drive the REAL bin/fm-pr-merge.sh
# with a stubbed forge (gh) and merge tool (gh-axi), one scenario per verdict.
set -u
ROOT=/home/nikita/.no-mistakes/worktrees/d9575cf3415e/01KZWWDSMSBGGT792YQ5ENAAWV
PR_MERGE="$ROOT/bin/fm-pr-merge.sh"
BASE=$(mktemp -d "${TMPDIR:-/tmp}/fm-merge-demo.XXXXXX")
trap 'rm -rf "$BASE"' EXIT

setup() {  # $1 = case name -> echoes case dir
  local d="$BASE/$1" fake="$BASE/$1/fakebin"
  mkdir -p "$d/state" "$fake" "$d/wt"
  cat > "$d/state/task-x1.meta" <<M
window=fm-task-x1
worktree=$d/wt
project=$d/project
kind=ship
mode=no-mistakes
M
  # gh stub: pr view rollup + merge_group runs, driven by env fixtures.
  cat > "$fake/gh" <<'SH'
#!/usr/bin/env bash
case " $* " in *headRefOid*) echo deadbeefcafefeed0000000000000000deadbeef; exit 0;; esac
case "${1:-} ${2:-}" in
  "pr view")
    [ "${FM_TEST_CHECKS_FAIL:-0}" = 1 ] && exit 1
    printf '{"number":%s,"baseRefName":"main","statusCheckRollup":%s}\n' "${FM_TEST_PR_NUMBER:-$3}" "${FM_TEST_ROLLUP:-[]}"; exit 0;;
esac
case "${1:-}" in
  api) [ "${FM_TEST_CHECKS_FAIL:-0}" = 2 ] && exit 1
       printf '{"workflow_runs":%s}\n' "${FM_TEST_QUEUE_RUNS:-[]}"; exit 0;;
esac
exit 0
SH
  cat > "$fake/gh-axi" <<'SH'
#!/usr/bin/env bash
echo "  >> gh-axi $*  (MERGE EXECUTED)" >&2
exit 0
SH
  chmod +x "$fake/gh" "$fake/gh-axi"
  echo "$d"
}

drive() {  # $1 case dir ; rest = args
  local d=$1; shift
  FM_ROOT_OVERRIDE="$ROOT" FM_STATE_OVERRIDE="$d/state" \
    PATH="$d/fakebin:$PATH" "$PR_MERGE" "$@" 2>&1
  echo "  [exit $?]"
}

URL=https://github.com/nguzen/aln/pull/182
red='[{"__typename":"CheckRun","name":"e2e visual QA (Playwright)","status":"COMPLETED","conclusion":"FAILURE"}]'
green='[{"__typename":"CheckRun","name":"CI","status":"COMPLETED","conclusion":"SUCCESS"}]'
qred='[{"name":"CI","head_branch":"gh-readonly-queue/main/pr-182-abc","event":"merge_group","status":"completed","conclusion":"FAILURE","created_at":"2026-08-10T17:54:00Z","run_attempt":1}]'
qgreen='[{"name":"CI","head_branch":"gh-readonly-queue/main/pr-182-abc","event":"merge_group","status":"completed","conclusion":"SUCCESS","created_at":"2026-08-10T18:10:00Z","run_attempt":1}]'

echo "=== 1. RED branch check (the incident: e2e visual QA failed) ==="
d=$(setup c1); FM_TEST_ROLLUP="$red" drive "$d" task-x1 "$URL"
echo
echo "=== 2. Branch GREEN but MERGE-QUEUE RED (the 2026-08-10 blind spot) ==="
d=$(setup c2); FM_TEST_ROLLUP="$green" FM_TEST_QUEUE_RUNS="$qred" drive "$d" task-x1 "$URL"
echo
echo "=== 3. UNREADABLE (forge silence — queue read fails) — refuse, not pass ==="
d=$(setup c3); FM_TEST_ROLLUP="$green" FM_TEST_CHECKS_FAIL=2 drive "$d" task-x1 "$URL"
echo
echo "=== 4. --allow-failing-checks: deliberate red merge, announced on stderr ==="
d=$(setup c4); FM_TEST_ROLLUP="$red" drive "$d" task-x1 "$URL" --allow-failing-checks
echo
echo "=== 5. All GREEN (branch + newest queue attempt) — merge proceeds ==="
d=$(setup c5); FM_TEST_ROLLUP="$green" FM_TEST_QUEUE_RUNS="$qgreen" drive "$d" task-x1 "$URL"
echo
echo "=== 6. Unknown own flag — usage error, exit 2, never forwarded to gh-axi ==="
d=$(setup c6); FM_TEST_ROLLUP="$green" drive "$d" task-x1 "$URL" --force

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

✅ **Review** - passed

✅ No issues found.

✅ **Test** - passed

✅ No issues found.

  • bash tests/fm-pr-merge.test.sh — 17/17 pass (red branch, red merge queue w/ green branch, superseded queue attempt, unreadable rollup, unreadable queue, foreign-PR payload, --allow-failing-checks, unknown own flag)
  • bash tests/fm-pr-check-security.test.sh — exit 0, no failures (regression: gh mock taught the two new reads with green defaults)
  • End-to-end CLI transcript driving the real bin/fm-pr-merge.sh against a stubbed gh/gh-axi for all six operator-visible verdicts: /tmp/no-mistakes-evidence/01KZWWDSMSBGGT792YQ5ENAAWV/demo.sh
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

…eadable

bin/fm-pr-merge.sh called gh-axi pr merge without ever reading the forge's
check state, so "never merge a red PR" lived only in AGENTS.md and depended on
session attention. On 2026-08-10 that gap shipped a broken commit: the
merge-queue run for nguzen/aln pull request 182 failed at 17:54 UTC, the pull
request was merged at 17:59 UTC, and the defect in it blocked every production
deploy until 2026-08-12.

The merge path now reads the verdict itself through the new
bin/fm-pr-checks-lib.sh and refuses before recording any state:

  * failing    at least one check the forge reports as failed
  * unreadable the state could not be established - never treated as a pass

Two sources are read, because either can be red alone: the pull request head's
status check rollup, and the newest merge-queue attempt, whose checks run again
on a combined commit published as gh-readonly-queue/<base>/pr-<n>-<sha>. Only
the newest queue attempt is judged, so a superseded red attempt cannot block a
re-queued green one. `gh pr checks` is deliberately unused: it exits non-zero
both when a check failed and when one is still running, so its exit status
cannot classify a result.

--allow-failing-checks is the single deliberate override, and it announces
itself; AGENTS.md records that using it is a red merge and needs the same
explicit captain instruction any red merge needs.

Verified against the live incident: the library classifies nguzen/aln#182 as
failing (branch and merge-queue) and kunchenguid#183 as green.

Tests: tests/fm-pr-merge.test.sh gains the red-branch, red-merge-queue-with-
green-branch, superseded-attempt, unreadable, wrong-payload, override and
unknown-flag cases; both guards were mutation-checked (blinding the queue read
fails only the queue case, accepting an unreadable verdict fails only the
unreadable case).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant