Skip to content

feat(review): merge-train gate for FIFO merge ordering #4004

Description

@JSONbored

Context

The review stack is supposed to process PRs FIFO but does not — it merges whichever PR's own gate clears first, with zero awareness of older sibling PRs. Confirmed empirically via a read-only production query joining pull_requests against itself:

SELECT a.repo_full_name, COUNT(DISTINCT a.number) AS distinct_newer_prs_that_jumped_ahead
FROM pull_requests a JOIN pull_requests b ON a.repo_full_name = b.repo_full_name
WHERE a.state='closed' AND a.merged_at IS NOT NULL AND b.state='closed' AND b.merged_at IS NOT NULL
  AND b.number < a.number AND b.created_at < a.created_at AND b.merged_at > a.merged_at
GROUP BY a.repo_full_name

Result: 1,364 (awesome-claude), 708 (metagraphed), 505 (gittensory) distinct PRs merged while an older, still-open-or-later-merging PR was pending.

Root cause: mergePullRequest (src/github/pr-actions.ts:91) and its sole caller executeAgentMaintenanceActions's "merge" case (src/services/agent-action-executor.ts:820) run a 9-step gate stack that is entirely scoped to one PR — no step reads any sibling PR's age or state. The two mechanisms that look like they'd help don't: src/selfhost/queue-fairness.ts's backlog/fresh ratio governs job-claim order (not merge order) and can process multiple same-repo PRs concurrently with no ordering constraint; src/settings/agent-sweep.ts's regateSweepOrderMode (#3815) only changes which stale PR the cron re-reviews first — its own issue body says explicitly "a sort-key change alone cannot make cross-path ordering fully deterministic."

This causes real merge conflicts: an older PR touching the same files as a just-merged newer one goes stale, and nothing proactively re-checks it — it sits until an unrelated webhook or the next sweep cycle touches it.

Requirements

  1. A new pure, unit-testable module src/review/merge-train.ts exposing a shouldWaitForOlderSiblings(thisPrNumber, thisPrCreatedAt, siblings, nowMs) predicate: true when an older, still-viable (not conflicted, not past a staleness cap) sibling PR exists in the same repo.
  2. A staleness/escape-hatch cap (MERGE_TRAIN_MAX_WAIT_MS, mirroring isRegateSweepDraining's time-boxed pattern) so one stuck old PR can never block newer ones forever.
  3. A conflicted (mergeableState === "dirty") older sibling must never block — it's not "about to merge," it's stuck.
  4. Wire the gate into executeAgentMaintenanceActions's "merge" case, scoped to action.actionClass === "merge" only, using the sibling-PR list already fetched via listOtherOpenPullRequests (already ordered ascending by PR number) — no new query, no new persisted state needed for the core algorithm.
  5. Gate the whole feature behind a new per-repo settings field (e.g. mergeTrainMode: "off" | "audit" | "enforce", default "off"), following the exact template used for regateSweepOrderMode (migration + resolver + openapi + .gittensory.yml parity, same PR).
  6. Ship in "audit" mode first (log "would wait for #X" without actually blocking) on one repo for a burn-in period before flipping any repo to "enforce", mirroring auto-apply.ts's existing shadow-soak-then-promote pattern.
  7. Full test coverage per house style (99% patch, both branches of every conditional), including the tie-break/cycle-avoidance case (guard with PR number, same as existing precedent elsewhere in this codebase).

Deliverables

  • src/review/merge-train.ts (new, pure).
  • Wiring in src/services/agent-action-executor.ts + src/queue/processors.ts (threading otherOpenPullRequests/createdAt into the executor context — already fetched at existing executeAgentMaintenanceActions( call sites).
  • Migration adding merge_train_mode (or equivalent) to repository_settings, plus resolver/openapi/.gittensory.yml schema entries.
  • Tests mirroring agent-sweep.test.ts's convergence-test style, plus an audit-event addition (merge_train_deferred or similar) so a held merge is visible in the per-PR audit timeline.

Expected outcome

Within a repo with mergeTrainMode: "enforce", PRs merge in a strict oldest-viable-first order (bounded by the staleness cap), eliminating the out-of-order-merge-conflict pattern proven above. "audit" mode gives a before/after count of how often the gate would have fired, without changing any behavior, to validate the fix before turning it on for real.

Non-goals

  • Not a full GitHub-native-merge-queue / Mergify / Bors-style staging-branch reimplementation — this is a lightweight admission check at the existing merge call site, consistent with this codebase's "advisory, fail-open, defense-in-depth" lock philosophy. update_branch (already implemented) is the natural next lever if a stronger integration-branch guarantee is wanted later.
  • Does not change queue-fairness.ts or regateSweepOrderMode — both are correctly solving different, narrower problems and should be left alone.

Metadata

Metadata

Assignees

Labels

maintainer-onlyOwner-only work — yields no Gittensor points.orbGittensory Orb related - maintainer self-hosting analytics.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions