SPAR-348: Wait for GitHub Actions check-runs before merging (v2.0.0)

[sskirby]

What & why

Jira: SPAR-348

/integrate only waited on the legacy combined commit-status API (Buildkite), which is blind to GitHub Actions check-runs. In the nulogy/spark monorepo that let a SensrTrxMES/-only PR merge as soon as Buildkite's no-op went green, without waiting on the SFac suite (test/client_test/e2e). Branch protection has no required checks either.

Approach (v2.0.0)

Gate /integrate on a commit's checks read via GitHub GraphQL statusCheckRollup, which returns legacy status contexts (e.g. buildkite/packmanager) and Actions check-runs together — so both CI systems are gated through one uniform path. normalize_rollup maps every context into a {name,status,conclusion} shape; evaluation is by latest run per name (by check-run id), failing on any conclusion outside success/neutral/skipped.

Config is one generic env, REQUIRED_CHECKS — a JSON array of {paths?, checks} rules. The action carries no product knowledge; the consuming repo pairs its own paths with its own check/status names:

[ {"paths":["PackManager/"],"checks":["buildkite/packmanager"]},
  {"paths":["SensrTrxMES/"],"checks":["test","client_test","e2e"]} ]

A rule with no paths always applies; a path-scoped rule applies only when the PR changes a matching prefix. The action always requires every check present on the commit to pass; the named checks additionally must be present, which anchors the wait so a PR can't merge in the window before its checks register. New checks are caught by the "all present" rule with no config change — you only name one reliably-running check per product as an anchor.

Robustness (all from adversarial verification passes)

• set -e-safe throughout; transient GraphQL failures retry, not abort.
• Wall-clock timeout (CI_WAIT_TIMEOUT_SECONDS, default 4h) — never hangs to the job's 6h cap.
• Merge is sha-guarded (sha=$HEAD_BRANCH_HEAD) so only the CI-validated commit lands; redundant post-CI rebase removed.
• Empty-config mode won't merge on an empty check set (waits for ≥1 to appear); malformed REQUIRED_CHECKS rules are skipped, not fatal; a commit with >100 checks is refused (no partial-view merge).
• No more special-cased "legacy status must be success" gate — so no pure-Actions-repo hang.

Versioning (breaking → v2.0.0)

• v1.1.1 tag = old status-only behavior (pin to keep it).
• entrypoint.sh version banner bumped; v2.0.0 to be tagged after review + verification.

Testing

• 53 ci_checks.sh unit assertions (normalize/rollup-validity, latest-per-name, required/all-present, reruns, null-name).
• Live GraphQL query against real SFac + PackManager commits (both normalize correctly).
• set -e harnesses driving the real config parser, pr_touches_paths (fail-closed), the empty-mode floor, and the >100 guard.
• Three adversarial verification passes; every confirmed finding fixed (the last round caught the empty-config early-merge + a malformed-config abort).
• End-to-end on throwaway spark PRs pending (via the draft pin PR nulogy/spark#74).

🤖 Generated with Claude Code

https://claude.ai/code/session_01CUQaePmRdFCZU5tNqBdTps

[sskirby]

Findings from a deeper review pass (4 independent adversarial checks + synthesis)

While answering the sleep 45 question I ran a fuller review of the CI-wait loop against the live GitHub/Buildkite behavior on spark. Summary + proposed scope split below.

Confirmed: the settle was inert for spark (fixed in d5300a8)

Empirically, /status stays pending for 7.6–9.1 min (Buildkite holds its pending status until the build finishes) while SFac check-runs register in ~5–10s. The pending guard holds the loop until check-runs are long present; 10s vs 45s makes no difference. Reverted to the original 10s cadence.

The real residual risk (not the settle): vacuous empty-check-runs gate

check_runs_incomplete_count returns 0 for an empty set, so "SFac check-runs haven't registered yet" is indistinguishable from "SFac passed." If a GitHub Actions incident (or a dropped synchronize webhook) delayed check-run creation past Buildkite's terminal status, /integrate would merge with SFac never gated. The settle never covered this (it's minutes, not seconds). Under normal spark timing this never fires, but it's the design's true early-merge hole.

Findings & proposed scope

#	Finding	Severity	Proposed
1	sleep 45 inert for spark	—	✅ fixed (d5300a8)
2	No wall-clock timeout → a stuck check-run (newly gated by this PR) or a Buildkite status that never goes terminal hangs the loop to the 6h job timeout	medium	this PR
3	New jq helpers abort the whole integration under set -e if /check-runs returns an error payload (rate-limit/502) — a regression vs v1.1.1, which only did jq -r .state. Fail-closed (no bad merge) but kills a good run	medium	this PR (treat non-array payload as "keep polling")
4	Pure-Actions repos (no legacy status) can never merge under v2.0.0 — /status stays pending/0 forever. Not a spark issue (always has buildkite/packmanager), but the v2.0.0 framing implies general Actions support	high (doc)	this PR (document the "must have a pending-holding legacy status" requirement)
5	Vacuous empty/partial gate (above): make a positive assertion that expected checks (test/client_test/e2e) are present before trusting a complete result; also paginate (>30 runs)	medium	follow-up
6	cancelled / action_required treated as failures → a concurrency-auto-cancelled run would abort a green integration. Not reachable on spark today (SFac workflows have no concurrency: block)	medium	follow-up
7	Post-CI second git rebase + merge with no sha guard. Currently a no-op (no second git fetch), so latent, and pre-existing (identical in v1.1.1)	low	follow-up

Proposed: fix 2, 3, 4 here (small, tightly coupled to this change); file 5, 6, 7 as a follow-up story under SPAR-151.

[sskirby]

Update: findings 2–7 implemented, plus a self-review pass

All the follow-ups from the findings comment are now in this PR, and a second adversarial pass (read + execute the code) surfaced a high-severity set -e bug that's also fixed. Pushed through 03a0a15.

Implemented

• Wall-clock timeout CI_WAIT_TIMEOUT_SECONDS (default 4h — sized above the real ~2.6h SFac e2e, below the job's 6h cap).
• REQUIRED_CHECK_RUNS (+ path-scoped REQUIRED_CHECK_RUNS_PATHS): require a named set of checks to be present and pass, closing the "empty check-run set = passed" hole. Path scoping keeps PRs that don't touch the product from waiting on checks that never run.
• Latest run per name (by check-run id, then started_at): a rerun / concurrency re-create supersedes the earlier run — fixes both stale-cancelled masking and cancelled/action_required false-negatives on advisory runs.
• Merge with sha=$HEAD_BRANCH_HEAD so only the CI-validated commit lands; dropped the redundant post-CI rebase.
• README: full env docs + the legacy-status requirement (pure-Actions repos need at least one pending-holding status).

Self-review pass found & fixed (confirmed by executing the code under real bash)

• HIGH — a transient non-JSON /status body or a curl transport failure aborted the whole integration under set -e (the retry path only covered JSON error bodies). Both curls are now || { … continue; } + --max-time, and status parses with a .state // "null" fallback → transient blips retry instead of cancelling an hours-long integration.
• MEDIUM — pr_touches_paths now retries and fails closed (undeterminable scope → enforce required checks) instead of silently downgrading to the weaker all-present gate.
• MEDIUM — REQUIRED_CHECK_RUNS entries are whitespace-trimmed ("test, e2e" no longer hangs to the timeout reporting present checks as "missing").
• LOW — null-started_at newer runs no longer masked (id-primary ordering); null check-run names filtered before keying (can't crash the gate).

Verification: 41 ci_checks.sh unit assertions (incl. latest-per-name, whitespace, null-name, reruns), shellcheck clean on changed code, bash -n clean, and set -e repros of the abort scenarios + a pr_touches_paths harness (stubbed curl) driven against the real functions.

Dismissed as non-bugs: Mode-A waiting on advisory checks (intended v2.0.0 behavior; REQUIRED_CHECK_RUNS is the escape hatch) and Mode-B waiting when a required check legitimately never runs (by-design fail-closed; keep REQUIRED_CHECK_RUNS_PATHS a subset of each check's trigger paths).