Skip to content

feat(ci): detect PR queue collisions GitHub cannot see - #75

Merged
pai-scaffolde merged 3 commits into
mainfrom
fix/sca-4638-merge-order-sweep
Aug 20, 2026
Merged

feat(ci): detect PR queue collisions GitHub cannot see#75
pai-scaffolde merged 3 commits into
mainfrom
fix/sca-4638-merge-order-sweep

Conversation

@pai-scaffolde

Copy link
Copy Markdown
Collaborator

Closes the SCA-4638 gap: scaffolde-ai got a merge-order collision sweep (SCA-4633); this repository had none.

The gap

GitHub's mergeStateStatus is a per-PR measurement against that PR's own base. It is never a measurement of the queue. On 2026-08-10 six PRs all reported CLEAN and the queue could not land in any order, while four Paperclip issues sat in_review waiting on a merge path that had been broken for days. A queue that cannot land reads exactly like a queue awaiting review.

Live evidence — the sweep run against this repo's current queue

Probed 19 open PRs: #50 #51 #52 #54 #58 #61 #62 #63 #64 #65 #66 #67 #68 #69 #70 #71 #72 #73 #74

Stacked (hard ordering edge, not a conflict):
  #62 sits on top of #66      #64 sits on top of #63     #65 sits on top of #64
  #66 sits on top of #65      #67 sits on top of #66     #71 sits on top of #69
  #72 sits on top of #71      #74 sits on top of #73

MUTUALLY CONFLICTING — decide which lands first, then rebase or resolve the other:
  #50 <-> #63
      agent/lsp/manager.py
      hermes_cli/config_defaults.py
      website/docs/user-guide/features/lsp.md
  ... 16 mutually-conflicting pairs in total

Viable merge order:
  1. #54   2. #61   3. #68   4. #73   5. #74

Held back pending a decision: #50 #58 #62 #63 #64 #65 #66 #67 #69 #70 #71 #72
No verdict (held out of the order): #51 #52

#50 and #63 both report CLEAN on the board. Three conflicting files, zero warning from CI or the PR list. That is the whole gap in one pair.

Two decisions carry the work

Ancestry gates stacking. baseRefName records what a branch was opened against, and keeps saying so after a rebase or force-push moves the child off its parent's head. A sweep that believes it marks the diverged pair "same stack" and skips the collision probe entirely — the shape that renders as a tidy linear stack on the board and is invisible to everything else. Every declared link is confirmed with git merge-base --is-ancestor; unconfirmed links are reported and the pair is probed like any other siblings.

The mirror case is caught too: a PR targeting main whose head already contains another open PR gets an ordering edge, because landing it first would silently merge that PR's work under a different review.

Merge-commit semantics, not squash. This repository takes merge commits. A --squash simulation re-applies a parent's hunks as a flat diff against a base that already contains them and reports collisions that do not exist — the false positive that had to be discarded during the manual sweep. test_merge_commit_probe_is_clean_where_a_squash_simulation_false_positives pins the choice by showing both semantics disagreeing on the same pair, rather than leaving it as a comment.

Acceptance criteria

# Criterion Where
1 A sweep exists and runs on a schedule .github/workflows/pr-merge-order.yml, cron 20 */6 * * * + workflow_dispatch
2 Positive control: flags a sibling pair GitHub reports CLEAN test_sibling_collision_is_flagged_though_each_merges_clean_alone, asserting preflight is clean for both first
3 Merge-commit semantics, with a test stating why squash is wrong test_merge_commit_probe_is_clean_where_a_squash_simulation_false_positives
4 The verdict reaches a human on collision Job summary every run; opens or appends to a [pr-merge-order] issue on exit 1

On the false-green failure mode

The ledger's recurring defect is a guard that passes by doing nothing. Three things are aimed at it:

  • Paired controls. Every positive control has a negative twin, so neither can pass by a constant verdict.
  • Tamper-verified. Neutering the ancestry gate to if True fails test_diverged_stack_is_probed_instead_of_skipped; forcing the probe to always report clean fails that and the sibling-collision control. Both were run; both failed as intended; both restored.
  • Exit 2 fails the job. A sweep that could not run is not a clean queue.

Tests run against real git repositories built in a tmpdir, not stub probes — the failure under test only exists in actual three-way merges.

Safety

Read-only with respect to the queue and the working tree: no checkout, no merge, no branch write. Heads are fetched into refs/pr-merge-order/<n> (pruned each run, so the namespace cannot grow unbounded and hold dead objects), and merge-tree writes only loose objects — it stays safe to run during a disk-pressure outage, which is exactly when the queue is frozen and the answer is needed.

One porting note worth flagging: the repo slug is pinned to a named remote rather than left to gh. This checkout carries origin/pai-scaffolde alongside upstream (NousResearch), and with no default set gh pr list resolves to upstream and returns its ~85,000-PR queue. A sweep that trusted that resolution would report an order for PRs this repository never lands.

Verification

  • pytest tests/scripts/test_pr_merge_order.py — 26 passed
  • ruff check scripts/pr_merge_order.py tests/scripts/test_pr_merge_order.py — All checks passed
  • Live run against the current 19-PR queue — exit 1, output above
  • Workflow YAML parses; fetch-depth: 0 (a shallow clone makes every ancestry answer wrong and would silently disable the divergence check)

The 2026-08-10 collisions were already resolved by hand. This is the durable detector, not that repair.

Refs: SCA-4638, SCA-4633

GitHub's `mergeStateStatus` measures each PR against its own base, never
against the other open PRs. On 2026-08-10 six PRs all reported CLEAN and
the queue could not land in any order, while four issues sat in review
waiting on a merge path that had been broken for days. A queue that
cannot land reads exactly like a queue awaiting review.

`scripts/pr_merge_order.py` simulates the real three-way merge of every
candidate pair with `git merge-tree --write-tree` and reports a landable
order, or names the colliding pair and the files.

Two decisions carry the work:

Ancestry gates stacking. `baseRefName` records what a branch was opened
against and keeps saying so after a rebase or force-push moves the child
off its parent's head. Believing it marks a diverged pair "same stack"
and skips the collision probe — the shape that renders as a tidy linear
stack on the board. Every declared link is confirmed with `git merge-base
--is-ancestor`; an unconfirmed one is reported and the pair is probed.
The mirror case is caught too: a child targeting the trunk whose head
already contains another open PR gets an ordering edge, because landing
it first would merge that PR's work under a different review.

Merge-commit semantics, not squash. This repository takes merge commits.
A squash simulation re-applies a parent's hunks as a flat diff against a
base that already has them and reports collisions that do not exist —
the false positive that had to be discarded during the manual sweep. A
test pins the choice by showing both semantics disagreeing on one pair.

The sweep is read-only: no checkout, no merge, no branch write. Heads go
to `refs/pr-merge-order/<n>`, pruned each run so the namespace cannot
grow unbounded, and `merge-tree` writes only loose objects — so it stays
safe to run while the queue is frozen.

The repo slug is pinned to a named remote rather than left to `gh`. This
checkout carries origin/pai-scaffolde alongside upstream NousResearch,
and with no default set `gh pr list` resolves to upstream and returns a
queue this repository never lands.

Tests run against real git repositories rather than stub probes, since
the failure under test only exists in actual three-way merges. Each
positive control is paired with a negative one so neither can pass by a
constant verdict, and the wiring tests fail if the workflow stops
listening.

A detector nothing listens to is not a fix: the verdict reaches the job
summary every run and opens or updates a GitHub issue on collision. A
sweep that cannot complete fails the job rather than reporting a clean
queue.

Refs: SCA-4638, SCA-4633

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b84ca8da69

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/pr_merge_order.py Outdated
Comment thread .github/workflows/pr-merge-order.yml Outdated
Comment thread tests/scripts/test_pr_merge_order.py
Comment thread scripts/pr_merge_order.py
Comment thread scripts/pr_merge_order.py Outdated
Comment thread scripts/pr_merge_order.py
Comment thread .github/workflows/pr-merge-order.yml Outdated
Comment thread scripts/pr_merge_order.py
Comment thread scripts/pr_merge_order.py Outdated
Addresses the nine review findings on #75. The load-bearing one is a false
green in the tool built to end false greens: the exit expression read
`mutual_conflicts or diverged`, so a run in which NO PR was ever measured
returned 0 and announced a landable queue. Positive control, with every probe
raising: 3 PRs, 0 verdicts, exit 0.

`exit_code_for` now derives the status from coverage — 2 when any PR has no
verdict, 1 when everything was measured and some PR has no landing order, 0
only when every PR is in the order. `diverged` is no longer a status of its
own: a diverged stack is re-probed as siblings, so it either produces real
verdicts or lands in `unverified`, and failing on the divergence itself
reported an un-landable queue for stacks the pair probes had already
sequenced cleanly.

Running that against the live queue caught a regression in the fix: a PR that
fails preflight is DIRTY, which is a measured verdict, not a failure to
measure. Filing it under `unverified` made the sweep exit 2 — job red,
collision issue suppressed — on every run while any DIRTY PR was open. Split
into `unmergeable` (blocks, exit 1) vs `unverified` (exit 2). The live queue
now exits 1 with #51/#52 held for a stated reason instead of exit 2.

Also from the review:

- Probe commits get an explicit identity. `commit-tree` exits 128 "Author
  identity unknown" where actions/checkout leaves no user.name/user.email;
  every pair probe would raise, and combined with the old exit expression the
  sweep would report a landable queue having merged nothing.
- The status -> outcome decision moves to scripts/ci/pr_merge_order_gate.sh so
  the tests execute it instead of grepping the workflow for substrings. Those
  assertions passed regardless of correct wiring and broke on reformatting —
  one of them failed on this very change while the behaviour improved. The
  gate accepts only 0/1 and fails the job for 2, 127, 137 and anything else.
- The candidate order is replayed onto a cumulatively advancing base. Pairwise
  probes each start from the trunk plus ONE landed PR, so they verify a proxy;
  the replay measures the claim the report actually makes. A rejection
  recomputes the order without the culprit, bounded and terminating.
- A queue truncated by `--limit` is refused rather than answered, since a
  collision involving an omitted PR would otherwise read as clean.
- Blocked state propagates across the full ancestry DAG. The scalar
  `stacked_on` keeps only the last parent, so a head containing two open PRs
  could reach merge_order still carrying a forgotten ancestor's commits.
- The empty-queue early return prunes probe refs, which it previously skipped
  exactly when there was most to collect.

Verification: 47 tests pass (was 26), shellcheck and ruff clean, workflow YAML
parses, live sweep against the 20-PR queue exits 1 with a replay-verified
order.

Note: I could not reproduce the pairwise-clean/cumulatively-conflicting triple
the replay finding describes — 20+ line geometries plus rename and add/add
families all stayed consistent. The replay is kept because it measures the
report's actual claim rather than a proxy, and a replay failure is by
construction a real conflict on the real landing sequence.
@pai-scaffolde

Copy link
Copy Markdown
Collaborator Author

All nine findings dispositioned — eight adopted, one adopted with a correction to its premise

Pushed as 81ee1989e1. Eight were real and are fixed; the ninth I kept but could not reproduce, and I am saying so rather than claiming a counterexample I do not have.

The sharpest one is a false green in the tool built to end false greens, so I built a positive control before touching it:

3 PRs, every probe raising (the clean-checkout condition):
  unverified       : [1, 2, 3]
  merge_order      : []
  mutual_conflicts : []
  >>> exit code    : 0        <- "the queue has a landable order"

Zero verdicts established, exit 0. That is exactly the shape this sweep exists to catch, and it was reachable through commit-tree failing on a fresh runner (finding 5), which makes findings 1 and 5 a single composite failure rather than two independent nits.

Disposition

# Finding Disposition
P1 Exit ignores unverified/blocked Adopted. exit_code_for: 2 = a PR has no verdict, 1 = fully measured with no landing order, 0 = every PR in the order. diverged dropped as a status of its own — it is a reason to probe, not a verdict.
P1 commit-tree has no identity Adopted. Explicit GIT_AUTHOR_*/GIT_COMMITTER_* for the disposable probe commits, and git's own stderr is now carried into the error instead of "failed to synthesize".
P1 Workflow accepts statuses outside the contract Adopted. Only 0/1 pass; 2, 127, 137 and anything else fail the job.
P1 Tests read workflow YAML instead of executing it Adopted. Decision extracted to scripts/ci/pr_merge_order_gate.sh; tests execute it.
P1 Mutable actions/checkout@v4 Adopted. Pinned to the SHA the other 32 usages in this repo already use.
P1 Replay the order cumulatively Adopted, premise unconfirmed — see below.
P2 --limit truncation Adopted. A truncated queue is refused, not answered.
P2 Scalar stacked_on loses DAG parents Adopted. Blocked state propagates over the full ancestry closure.
P2 Empty queue skips prune_probe_refs Adopted. Pruned before the early return.

The exit-code fix caught a regression in itself

Running it against the live queue is what surfaced this, not the test suite. A PR that fails preflight is DIRTY — that is a measured verdict, not a failure to measure. My first cut filed it under unverified, which made the sweep exit 2 (job red, collision issue suppressed) on every run while any DIRTY PR was open:

before the split:  EXIT=2   "No verdict (held out): #51 #52"
after the split:   EXIT=1   #51/#52 held back with their measured reason

Split into unmergeable (real verdict, blocks, exit 1) vs unverified (no verdict, exit 2). Both directions are now pinned by tests, including test_a_dirty_pr_does_not_mask_a_landable_queue.

On the cumulative-replay finding: adopted, but I could not reproduce your case

You reported reproducing three branches whose pair probes were all clean but which conflict when merged cumulatively. I could not construct that, and I tried specifically to:

  • a 20+ configuration sweep of single-file hunk geometry (varying the gap between A's, B's and C's edits, including adjacent and coalescing distances)
  • rename + far edits, rename + adjacent edits, add/add with identical content

In every case where the cumulative replay conflicted, at least one pair probe also conflicted and would already have been caught. With a shared merge base, pairwise-clean appears to imply no textual overlap for these families.

I have kept the replay anyway, for a reason I can defend rather than the one in the finding:

  1. It measures the claim the report actually makes ("this order lands"), where the pairwise pass measures a proxy.
  2. A replay rejection can never be a false positive — it is a conflict observed on the real landing sequence.
  3. It is linear on top of an already-quadratic pass, so it costs approximately nothing.

If you still have the reproduction, I would genuinely like it — it would become a test. As it stands the replay is justified by what it measures, not by a counterexample I can show.

Verification

  • 47 tests pass (was 26). New coverage: the exit contract in all four states, the gate executed at statuses 0/1/2/127/137, replay positive and negative controls, probe identity with user.name/user.email unset, truncation refusal with a paired healthy-queue control, ancestry-DAG closure incl. cycle safety, and pruning on the empty path.
  • shellcheck clean, ruff clean, workflow YAML parses.
  • Live sweep against the 20-PR queue: exit 1, replay-verified order #54 → #61 → #68 → #73 → #74 → #75, 14 held back with stated reasons.

One test worth calling out: test_sweep_failure_is_not_reported_as_a_clean_queue failed on this change while the behaviour got better — it asserted on the literal string status" -eq 2. That is the substring-assertion finding demonstrating itself, which is why the replacement executes the gate instead.

@pai-scaffolde pai-scaffolde added the ci-reviewed CI-sensitive changes reviewed label Aug 20, 2026
@pai-scaffolde
pai-scaffolde merged commit 1bddd33 into main Aug 20, 2026
47 of 49 checks passed
@pai-scaffolde
pai-scaffolde deleted the fix/sca-4638-merge-order-sweep branch August 20, 2026 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-reviewed CI-sensitive changes reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant