Skip to content

MergeGate merges only a head that Review-E approved on GitHub (rc#2053)

This is part of rc#1976. It extends the check-run guard of rc#1028. Policy in Core, tests written first.

The bypass

MergeGate merges when three Valkey fields on merge-gate:{repo}#{pr} line up: review-approved, ci-passed, and no blocked. Two things set review-approved, and neither checks what GitHub says:

  1. POST /api/merge. Review-E's runtime decides that a review approved by searching the model's reply text: rig-agent-runtime src/stream-consumer.js:1182 sets approved = !changesRequested && (includes('approved') || includes('approve')), and changesRequested only matches three exact phrases. When it decides "approved" it calls POST /api/merge (Program.cs), which does only SetConditionAndTryMerge(repo, pr, "review-approved"). The route has no auth and no GitHub lookup, and it is reachable in-cluster and on NodePort 30080 (dashecorp/infra#448).
  2. The pull_request_review webhook. It sets the field for any reviewer login that contains review-e.

Nothing ever cleared the field: not on CHANGES_REQUESTED, not on a dismissal, and not on a push. Its 24 h TTL is renewed on every condition write. The rc#1028 guard in Merge() checked only check_runs. Reviews were checked only for task PRs in the one held repo (rc#2104). So an open PR with green CI and no approving review could merge in every other repo:

PR When What happened
rig-agent-runtime#677 (rc#2053) 2026-09-09 Merged 23 s after Review-E's CHANGES_REQUESTED, with zero approvals
rig-assistant#15 (rc#1976 comment) 2026-09-11 Merged 35 s after Review-E's CHANGES_REQUESTED, with zero approvals
rig-conductor#2341 (rc#1976 comment) 2026-09-23 Only COMMENTED reviews. /api/merge still set the flag and the 30 s CI-timeout path ran Merge(). The PR had already been admin-merged, so the only harm was a false MERGED event and an UNREVIEWED MERGE alert

The rule

Merge() is MergeGate's only way to GitHub's merge PUT, and every path runs the guard (NotReadyReasonForMergeAsync) before anything else:

Path into Merge() Reached from
All conditions met the review webhook, the check_run webhook (rc#2425: when its pull_requests is empty, the open PR is looked up by head SHA), POST /api/merge
"No required CI checks — merging now" an approval with no ci-passed on a repo without required checks
"CI timeout after 30s — merging anyway" an approval with no ci-passed on a repo with required checks
The recheck after a defer on running checks MergeGate itself: every 2 min, 15 passes at most (rc#2425)
The sweep (RetryAsync) MergeGateSweepService: every 5 min, for a hash with Review-E's approval and no merge recorded, at most 12 backed-off attempts per trigger (rc#2365)
ReleaseHeldAsync the signed cockpit release route
RecheckHeldAsync, RetryReleaseAsync the cockpit status route's nudge and retry

The guard reads the PR's head, its check_runs and its reviews from GitHub. The pure MergeGuardPolicy then decides. The PR may merge only when:

  • every non-skip check_run passed — AllRequiredChecksPolicy, unchanged; and
  • review-e-bot's latest verdict approves exactly the head — OperatorTaskSnapshotReading.ApprovalAtHead, the rule the release hold and the cockpit status route already use. Only the App's login review-e-bot[bot] counts: a human, another App, and the unregistered user login review-e-bot do not. The verdict is the latest APPROVED, CHANGES_REQUESTED or DISMISSED review; COMMENTED is not a verdict. It must be APPROVED with commit_id equal to the head, so an approval of an older head, including one from before GitHub's "update branch" merge commit, does not count.

The reviews are unreadable, never approved, when the GET fails, when the body is not a list, or when the Link header names a next page. GitHub lists reviews oldest first, 100 per page, so the first page alone can hold a stale approval.

Every decision logs one line with the review set it evaluated, as rc#2053 asked: [MergeGate] <repo>#<pr> guard at <sha8>: checks passed, review NotApproved [review-e-bot[bot]:CHANGES_REQUESTED@<sha8>], listing each review as login:STATE@sha8, oldest first, the newest ten.

What a defer does

Guard result Deferred reason MERGE_GATE_WAITING missing Field cleared
A check has not passed the check reason, e.g. pending: build (as before) CI ci-passed (as before)
Review-E has not approved the head review_not_at_head review review-approved
The reviews were unreadable review_lookup_failed review none: a GitHub fault is not evidence against the approval
Both the two reasons joined by ;, checks first CI, review both, by the rows above

Before this change, every defer cleared ci-passed and reported CI. A review defer now leaves a green ci-passed in place, so Review-E's approval webhook merges immediately. It no longer has to go through the "no ci-passed" paths. Clearing review-approved is what finally invalidates the stale flag (rc#1976, defect 1). The next real approval sets it again.

The PUT is pinned to the verified head

A push between the guard's read and the PUT would merge code nothing has checked. Two measures stop that:

  • Merge() compares the head of its title fetch with the head the guard verified. If they differ, it returns Deferred("head_moved_during_merge") and makes no PUT. That is a different token from the release route's 409 head_moved, which means the operator signed a head the PR has already left.
  • Every squash PUT sends sha = the verified head, so GitHub answers 409 instead of merging a head that moved in the last moment.

Before, only an operator-approved task PR sent sha. A task PR's approved head is the same verified head, so the release hold's pinning is unchanged.

A PR that is no longer open stops at the first read

The guard's GET /pulls/{n} also gives the PR's state. A PR GitHub does not report as open, or reports with no state, returns Deferred("pr_not_open") before any other read. It emits no MERGE_GATE_WAITING and clears nothing. rig-conductor#2341 shows why: it was admin-merged, then Review-E's late review reached POST /api/merge, and MergeGate appended a second MERGED. The token is the release route's 409 pr_not_open, with the same meaning.

The cockpit's recovery nudge no longer needs the flag

RecheckHeldAsync, the status route's nudge for a held task PR that Review-E approved but nothing held, used to run only while review-approved was set. The guard now drops that flag when its read shows no approval at the head. So a read that raced Review-E's approval would have switched off this recovery for good. The nudge runs only after the status route read that approval on GitHub, and the guard re-reads it, so it now needs only the absence of blocked.

What this does not change or cover

  • Held task PRs. The release hold still runs after the guard and applies the same review rule, so it reads the reviews a second time. Its outcomes are unchanged, because the guard reuses the hold's reason strings.
  • POST /api/pr-reviews/merge merged with no CI check, no review check and no auth, and it never entered MergeGate. Nothing called it. Deleted by #2358 (rc#2346); live it returns 404 (2026-09-24).
  • Merges outside the conductor. Conductor pin PRs (publish-image.yml, --admin), release-please PRs merged by Actions, refresh-github-cidrs.yml, and manual merges never reach MergeGate.
  • The runtime still matches text. Its /api/merge call can now only start a merge that Review-E actually approved on GitHub. Deleting the runtime's REVIEW_PASSED emit and ending the text match are the rig-agent-runtime follow-up (rc#2342's plan).
  • The rest of rc#1976. Still open: a required check that never registered a check_run (the guard sees only check_runs that exist), branch-protection probes hard-coded to main for stacked PRs, and an audit event for every conductor merge.

Nothing that should merge is lost

Measured on 2026-09-23 over the 481 PRs merged in the 29 non-archived dashecorp repos since 2026-08-24. Each was checked with gh api .../reviews against its merged head:

Class PRs Under this rule
Review-E's latest verdict approves exactly the merged head 311 merges, as before
Not merged by MergeGate: release-please PRs (github-actions), conductor pin PRs (publish-image.yml --admin), manual merges 162 untouched
MergeGate merges without that approval 8 deferred

Of the 8:

  • 4 should never have merged: rig-agent-runtime#677 and rig-assistant#15 (CHANGES_REQUESTED at the head), and dashecorp/infra#450 and rig-cockpit-worker#96 (a stale flag after a push that Review-E had not approved).
  • 2 merged 18 s and about 4 min before Review-E's approval of their head existed: rig-cockpit-worker#228 and rig-gitops#770. They would have merged on the approval webhook instead.
  • rig-agent-runtime#676 would have waited 76 s. Review-E approved its head, but the size gate's CHANGES_REQUESTED on an older commit came later, so the latest verdict was a rejection until Review-E approved again. GitHub's own reviewDecision reads it the same way, and the rule stays the one the release hold and status route share.
  • 1 is only possibly a MergeGate merge.

MergeGate, the pin job and the operator all merge as the same account (GITHUB_TOKEN is the operator's PAT). The classes above therefore come from branch deletion, timing, and a replay of the rc#1028 guard, not from merged_by. Every Review-E deployment authenticates as App 3068795, so its reviews are always review-e-bot[bot].

Tests

MergeGuardPolicyTests (Core, written first) cover:

  • the verdict for every review shape: non-approval states, a later rejection of the same head, an older head, other logins, no reviews, unreadable input and a next page;
  • the decision, with its reasons, missing conditions and cleared fields;
  • the three incidents, which must not be ready.

MergeGateTests drive the adapter through every non-approval shape, SetConditionAndTryMerge(..., "review-approved") (the /api/merge path), unreadable and paginated reviews, a head that moves after the guard, and the pinned PUT. In CockpitTaskEndpointsE2eTests, POST_api_merge_merges_only_a_head_Review_E_approved_on_GitHub runs the real host, route and MergeGate. A CHANGES_REQUESTED head is not merged, its flag is dropped and ci-passed stays. The same call merges once Review-E approves.

Mutation proof. Each mutation below compiled and turned tests red, and was reverted:

# Mutation Red
M1 Decide ignores the review verdict 18
M2 The guard treats every review as approved (the pre-fix gate) 12, incl. the e2e
M3 A Link: rel="next" is ignored 2
M4 Unreadable reviews also clear review-approved 3
M5 No head-moved check 1
M6 Only an operator approval pins the PUT (as before) 10
M7 A defer clears only ci-passed (as before) 10
M8 MERGE_GATE_WAITING always says CI (as before) 9
M9 A failed reviews GET counts as approved 1
M10 Any login containing review-e counts (the webhook's loose match) 3
M11 The verdict ignores which head was approved 3

Watch after deploy

  • kubectl logs -n rig-conductor deploy/rig-conductor-api | grep 'guard at' should show one line per merge attempt with the review set. The next Review-E approval should read checks passed, review Approved [...] and be followed by MERGED by rig-conductor.
  • A runtime /api/merge for a PR that Review-E did not approve logs merge guard says NOT READY (review_not_at_head) and no MERGED.
  • StuckWatcher ready_to_merge alerts: a PR stuck there after this deploy has no approval at its head, and it needs Review-E's approval, not a bypass.