Skip to content

fix(disposition): unify mergeable-state predicates across approve/hold/label with the merge gate, and surface merge-hold reasons on the PR #8758

Description

@JSONbored

Context

Root cause of the #8711 silent stall (parent: #8757). The four surfaces disagree on what a non-clean `mergeableState` means:

  • merge: requires `=== "clean"` — src/settings/agent-actions.ts:1010,1019
  • approve: only excludes `=== "dirty"` (`isConflict`, :942, applied :1289) → approves on unstable/blocked/behind/unknown while merge self-suppresses
  • `heldForManualReview` (:996-1001): no mergeableState term at all → no hold label, no hold comment
  • `manualHoldReason` fallback (:1081-1090): gated on `!reviewGood`, unreachable for green-gate+unstable
  • review-state label (:1170-1177): resolves to ready-to-merge for this exact state
  • the CORRECT reason ("merge withheld because mergeable_state is unstable") is already computed in `agentHoldAuditDetail` (src/queue/processors.ts:2265-2269) but only feeds `recordAuditEvent("agent.action.hold")` — never the PR.

Requirements

  • Introduce one shared predicate (e.g. `mergeableStateBlocksAction(state)`) and use it in the approve gate, `heldForManualReview`, and the review-state label ternary, matching the merge gate's own posture. Approve must not fire while merge would self-suppress on mergeable state.
  • When the ONLY thing preventing a would-merge is a non-clean mergeableState (gate passing, CI green, no other hold), the disposition must surface publicly: manual-review label + a comment naming the state and (when derivable from the CI aggregate, e.g. nonRequiredFailingDetails/advisoryCheckHold) the specific check(s) behind it. Reuse `agentHoldAuditDetail`'s wording; keep the audit-event write.
  • `"behind"` must NOT become a manual hold when the update-branch/rebase rail can already handle it — preserve the existing rebase-if-behind flow's precedence.
  • Executor: extend the live-recheck set (src/services/agent-action-executor.ts:439 area) so approve gets the same live mergeable-state re-verification merge already gets, closing the plan-vs-execute race.
  • No change to close semantics anywhere in this issue.

Deliverables

  • The shared predicate + all call sites converted.
  • Public hold surfacing (label + comment) for the merge-suppressed-on-mergeable-state case.
  • Approve live-recheck in the executor.
  • Regression test reproducing fix(review): keep a fetched file distinguishable from an omitted one under thin budget #8711's exact state (gate pass, required CI green, non-required third-party check action_required, mergeableState unstable): asserts NO approve, manual-review label applied, hold comment posted, no merge planned.

Test Coverage Requirements

99%+ patch coverage, branch-counted, both arms of every new conditional.

Boundaries

Trust-critical file — tightening-only: no path that today does NOT approve/merge may begin approving/merging as a result of this change.

Metadata

Metadata

Assignees

Labels

maintainer-onlyOwner-only work — yields no Gittensor points.

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions