Skip to content

fix(gate): an unstable merge state must not summon a human - #10123

Merged
JSONbored merged 1 commit into
mainfrom
fix/unstable-ci-no-manual-review
Jul 31, 2026
Merged

fix(gate): an unstable merge state must not summon a human#10123
JSONbored merged 1 commit into
mainfrom
fix/unstable-ci-no-manual-review

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Closes #10116

GitHub reports mergeable_state: "unstable" for two different things — a non-required check is failing, and checks are still running. The gate treated both as a manual-review hold, which on a repo meant to run autonomously is wrong in both cases: a pending PR is simply not ready yet, and a failing one has already been answered by CI. Neither wants a maintainer.

Observed on #10098 and JSONbored/metagraphed#8838. Neither was a guardrail hit; both sat labelled manual-review, and #10098 stayed open with finished (red) CI until a human stripped the label by hand.

Why it stuck

heldForManualReview was heldBy.length > 0 || unstableHolds. That OR is doing three things at once, none of them advisory:

  • the executor denies merge and approve while the label is present;
  • merge-train.ts:152 filters !sibling.heldForManualReview, so the PR is evicted from the train entirely — which is why these get skipped with no merge-train comment while other PRs merge past them;
  • section 1b's release condition (noManualReviewHoldWanted) also tested !mergeableStateUnstable.

That last one is the latch: applied while CI is red, then not liftable because CI is red.

Two things that look like escapes are not. releasedHolds contains only guardrailHit, so routing guardrail-path PRs to a stronger model with onCleanReview: proceed releases the guardrail term and nothing else — an unstableHolds PR stays held however the escalated review comes back. And guardrailEscalationCleared requires reviewGood, which requires green CI, so on a red-CI PR it cannot fire at all.

The change

An unstable state stops being a hold. It does not become mergeable.

before after
heldForManualReview true false
heldForUnstableMergeState true true (unchanged)
wouldApprove false (via the hold) false (via its own term)
wouldMerge false false (unchanged)
disposition label manual-review none
in the merge train evicted queued
  • heldForManualReview = heldBy.length > 0 — the declared MERGE_HOLD_INPUTS table becomes the whole definition, so heldBy and heldForManualReview finally agree. They did not before, which is how these holds reached the ledger with no cause under reason_code: "success".
  • wouldApprove gains an explicit && !unstableHolds. Load-bearing: it used to ride on the hold term, and dropping that without restating it here would let approve fire while merge self-suppressed — exactly fix(review): keep a fetched file distinguishable from an omitted one under thin budget #8711. Keyed on unstableHolds, not mergeable === "unstable", so fix(ops): scope the PagerDuty cooldown to rows that actually paged #9810's ignored-check dismissal stays approvable. (My first attempt used the blanket test and its own test caught it.)
  • Both label sites stop emitting. Section 1f (the merge-autonomy fallback) is removed outright, and section 2's ternary returns null — neither manual-review nor ready-to-merge, since promising "ready" on a PR the merge self-suppresses on is the other half of fix(review): keep a fetched file distinguishable from an omitted one under thin budget #8711. Removing only one would have fixed nothing in production: review_state_label is not in the Orb's autonomy map, so 1f is the site that actually fired.
  • !mergeableStateUnstable comes out of noManualReviewHoldWanted, releasing the latch on labels the planner applied.
  • mergeUnstableHoldReason / mergeUnstableHoldComment are deleted — with no hold there is no action to hang them on (there is no comment-only AgentActionClass).

The contributor still learns the state: the unified comment refuses "safe to merge" via mergeStateHeld, and the Checks tab names the culprits — always current, unlike a bot comment posted once per pass on every in-flight PR.

Verification

  • 377 tests across the three affected files; full suite and the whole check-* sweep green (dead-exports, dead-source-files, import-specifiers, checkers-wired, typecheck).
  • Mutation-tested — each guard inverted, confirming a test actually catches it:
mutation tests failed
restore || unstableHolds on heldForManualReview 4
drop && !unstableHolds from wouldApprove 3
let the label ternary fall through to ready-to-merge 2
restore !mergeableStateUnstable in the release condition 1
  • The two removed #8758 tests are rewritten rather than deleted, one per label site, so each removal is pinned independently.

Follow-up

#10117deriveUnifiedStatus still renders this state's comment as "Manual Review". That is text, blocks nothing, and the same "held" status serves dirty/behind plus five unrelated paths where a human genuinely is required, so retitling it needs a status-vocabulary decision rather than a one-liner here.

Confirmed against the production ledger

Not inferred from the code — read out of decision_records on the Orb.

#10098 (contributor PR, files under src/selfhost/**, matching no guardrail glob):

        created_at        | action | reason_code | reevaluation_reason   | findings_count
 2026-07-31T07:00:10.422Z | hold   | success     |                       |  0
 2026-07-31T07:01:04.832Z | hold   | success     | upstream_state_change |  0
 ... 9 more ...
 2026-07-31T07:33:02.614Z | hold   | success     | upstream_state_change |  0
(11 rows)

Eleven holds in 33 minutes, one per check update. reason_code: success with findings_count: 0 means the gate passed and reviewGood was true — which excludes the manualHoldReason fallback (it requires !reviewGood) and the guardrail path (guardrail_hold carries its own reason code). reviewGood && unstable is section 1f exactly. JSONbored/metagraphed#8838 shows the identical row.

The reason code is success because heldBy was empty while heldForManualReview was true — the disagreement this PR removes. Over the last week on the Orb:

 reason_code          | action | count
 success              | hold   |   565   <-- this bug
 missing_linked_issue | hold   |   503
 success              | merge  |   393
 guardrail_hold       | hold   |   327

success|hold is the largest hold bucket on the box, ahead of every named cause.

GitHub reports `mergeable_state: "unstable"` both for a failing non-required
check and for checks that are still running. The gate treated both as a
manual-review hold, so every PR whose CI had not finished got an enforcement
label: the executor denies merge and approve while it is present, and
merge-train.ts evicts a held sibling from the train entirely.

Section 1b's release condition also tested the same state, which made the label
a latch -- applied while CI was red, then not liftable because CI was red.
Observed on #10098, stuck until a human removed it by hand.

Neither escalation path could clear it: `releasedHolds` contains only
`guardrailHit`, and `guardrailEscalationCleared` requires green CI.

An unstable state no longer holds. It does not become mergeable either:
`wouldApprove` gains its own explicit `!unstableHolds` term (previously implied
by the hold), keyed on `unstableHolds` rather than the raw state so #9810's
ignored-check dismissal stays approvable, and `wouldMerge` already required
`clean`. Both label sites stop emitting -- the merge-autonomy fallback is
removed and the disposition ternary returns no label rather than falsely
claiming `ready-to-merge`.

Closes #10116
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Important

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏳ LoopOver is waiting…

LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting

@JSONbored JSONbored self-assigned this Jul 31, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.08%. Comparing base (ec259cb) to head (8cc7362).
⚠️ Report is 11 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10123      +/-   ##
==========================================
- Coverage   91.95%   91.08%   -0.88%     
==========================================
  Files         931      931              
  Lines      113921   113912       -9     
  Branches    27504    27498       -6     
==========================================
- Hits       104757   103754    -1003     
- Misses       7863     9052    +1189     
+ Partials     1301     1106     -195     
Flag Coverage Δ
backend 94.11% <100.00%> (-1.56%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/settings/agent-actions.ts 97.95% <100.00%> (-0.07%) ⬇️
src/settings/pr-disposition.ts 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@JSONbored
JSONbored merged commit 645b323 into main Jul 31, 2026
10 checks passed
@JSONbored
JSONbored deleted the fix/unstable-ci-no-manual-review branch July 31, 2026 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gate: an unstable merge state summons a human and then latches the manual-review label

1 participant