Skip to content

Merge-gate check_run race fix

Extended (2026-09-25, rc#2425). "The next check_run.completed webhook re-evaluates" did not hold for the last check, whose delivery could stop at a silent exit of the handler. See the late-check re-trigger.

Extended (2026-09-23, rc#2053). The guard described here now also requires review-e-bot's latest verdict to approve exactly the head, and it pins the squash PUT to that head. A review defer clears review-approved and reports review, not CI. See MergeGate merges only a head that Review-E approved on GitHub.

Tracking: rig-conductor#1028

What happened

PR #1023 (test fixture mock) merged at 2026-05-17T12:04:06Z while dotnet test was still running. dotnet test reported FAILURE at 12:04:09Z — three seconds too late. Main went red and required emergency revert PR #1030.

Trace of the race window:

Time (UTC) Event
11:59 PR #1023 opened
12:02:06 docs / docs-check passes → check_run.completed webhook → MergeGate sets ci-passed
12:04:05 review-e approves → MergeGate sets review-approved
12:04:05 SetConditionAndTryMerge sees both conditions met → calls Merge()
12:04:06 Merge API call succeeds, PR #1023 closed as merged
12:04:09 dotnet test reports FAILURE
12:04:10 Main is red

Root cause

Program.cs:1976-1997 (check_run.completed handler):

if (conclusion == "success")
{
    // ... dedup CI_PASSED emit ...
    await mergeGate.SetConditionAndTryMerge(repo, prNumber, "ci-passed");
}

The handler sets ci-passed=true on any successful check_run. There's no requirement that ALL required checks have passed — the first one wins.

When the PR has multiple required checks, the fastest one effectively decides the gate. For rig-conductor PRs, docs / docs-check (a YAML-frontmatter linter) finishes in ~30s, while dotnet test (Testcontainers Postgres + 1200+ tests) takes 2-3 minutes.

Fix

Centralised last-chance guard inside MergeGate.Merge():

private async Task Merge(string repo, int prNumber)
{
    var token = Environment.GetEnvironmentVariable("GITHUB_TOKEN");
    if (string.IsNullOrEmpty(token)) return;

    // rc#1028 — centralised guard. Every Merge() call site goes through here.
    var notReady = await NotReadyReasonForMergeAsync(repo, prNumber);
    if (notReady is not null)
    {
        Console.WriteLine($"[MergeGate] {repo}#{prNumber} merge guard says NOT READY ({notReady}) — deferring");
        await _redis.GetDatabase().HashDeleteAsync($"merge-gate:{repo}#{prNumber}", "ci-passed");
        await EmitEventAsync(repo, prNumber, "MERGE_GATE_WAITING", missing: ["CI"]);
        return;
    }

    // ... proceed with GitHub merge API call ...
}

NotReadyReasonForMergeAsync calls GET /repos/{owner}/{repo}/pulls/{n} for the head SHA, then GET /repos/{owner}/{repo}/commits/{sha}/check-runs?per_page=100, maps to AllRequiredChecksPolicy.CheckRun records, and consults the pure policy.

Why inside Merge() (not SetConditionAndTryMerge)

MergeGate.cs has three call sites for Merge():

  1. SetConditionAndTryMerge line 168 — the normal "all conditions met" path.
  2. Line 115 — reviewApproved=true + RepoHasRequiredChecksAsync returns false → immediate merge.
  3. Line 133 — reviewApproved=true + 30s CI-timeout → merge anyway.

The initial fix placed the guard at site 1 only; review-e (rc#1028 PR review) flagged that sites 2 and 3 bypass it. Moving the guard inside Merge() itself catches all three.

Fail-closed semantics

If the GitHub API call fails (network error, parsing error, token missing), NotReadyReasonForMergeAsync returns a non-null reason like "guard fetch failed: <ex.Message>". The merge is deferred. The next check_run.completed webhook re-evaluates.

Pure policy

ConductorE.Core.UseCases.AllRequiredChecksPolicy.NotReadyReason(checkRuns):

  • Returns null when every non-skip check is status=completed with conclusion in {success, skipped, neutral}.
  • Returns "pending: <names>" when any non-skip check is not yet status=completed.
  • Returns "failed: <name=conclusion>" when any non-skip check has a non-acceptable conclusion (failure, cancelled, timed_out, action_required, null).
  • Returns "only skip-list checks reported" when the only check_runs are in the skip list (Auto-merge orchestration, Forward-to-Discord, etc.).

22 unit tests in AllRequiredChecksPolicyTests cover the decision matrix.

What this does NOT fix

  • The check_run.completed handler at Program.cs:1976 still sets ci-passed on any single successful check. The handler-level cleanup is a future refactor; the safety net at Merge() time is sufficient as the immediate fix.
  • The window between the guard's GitHub API call and the actual merge API call is sub-second; a check_run failing in that window would still slip past. Acceptable per Closes #1028.
  • Branch protection required_status_checks on dashecorp/rig-conductor is still empty. A defense-in-depth OpenTofu PR in dashecorp/terraform could lock this down at the GitHub-server side; out of scope for this PR.

Three-tier test coverage

  • Pure (AllRequiredChecksPolicyTests, 22 tests): every status × conclusion combination plus the skip-list semantics plus the empty-list pathological case.
  • Adapter (NotReadyReasonForMergeAsync wrapper): NOT covered by automated tests. The GitHub HTTP call is ~50 lines of straightforward parsing + fail-closed try/catch; would need extracting behind a port (IPullRequestChecksReader) and stubbing. Tracking as a follow-up.
  • E2E: NOT covered. Would need a mock GitHub HTTP server. The integration boundary is validated by every live merge from this point forward — if the guard fires incorrectly, the fail-closed branch defers (no harm) and the next webhook re-evaluates.

Operational signals

A successful guard run logs nothing extra (Merge() proceeds as before). A deferred run logs:

[MergeGate] dashecorp/rig-conductor#NNN merge guard says NOT READY (pending: dotnet test) — deferring

and publishes a MergeGate guard deferred: <reason> event to logs:rig-conductor (visible in the dashboard log mirror).