From 65d8e0db3b2418314d21574f15350c666f6d9a33 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 08:02:30 -0700 Subject: [PATCH] =?UTF-8?q?fix(gate):=20say=20WHY=20a=20green,=20approved?= =?UTF-8?q?=20PR=20did=20not=20merge=20=E2=80=94=20surface=20GitHub's=20ow?= =?UTF-8?q?n=20refusal?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit JSONbored/loopover#9856: gate success, CI green, mergeable_state "clean", autonomy auto, approvals satisfied, the bot posted "LoopOver approves" and the panel said "Suggested Action - Approve/Merge · safe to merge" -- and it never merged. Recurring for weeks, across many PRs, with no stated reason. The cause was never a LoopOver bug. GitHub REFUSED the merge: merge forbidden (403): Merging stacked PRs via this API is not supported. Use the web interface instead. Stacked PRs cannot be merged through the REST API at all. The executor recorded the refusal against that head and correctly stopped retrying. But that reason is invisible to every surface: mergeable_state reads `clean`, no gate models it, the audit fell through to the residual "merge withheld because no merge action was planned", and the public comment kept promising a merge that cannot happen. Four distinct refusals across 7 PRs all rendered identically (403 stacked; three 405 repo-policy variants). Surface it in both places: - agentHoldAuditDetail reports GitHub's message when the block is for THIS head, checked last among the specifics (everything above it is a state LoopOver can still change on its own; this one only a human can). - the unified comment downgrades an otherwise-ready status to HELD and names the refusal in the verdict box, with the remedy ("merge it yourself"). Same downgrade-only discipline as the guardrail hold: a real gate/CI block still wins. A block recorded against a DIFFERENT head is stale and deliberately ignored -- that commit was never attempted, so claiming GitHub refused it would be a lie and would hide whatever is actually holding the new push. Clock discipline: the decision path uses the pass's recorded instant (decisionClock.nowMs), never a fresh Date.now() -- the #9492 invariant, whose guard caught a first attempt that violated it. The publish path matches on head SHA alone and explains why: it is a reporting surface with no decision clock, and merge_blocked_until bounds when LoopOver may RETRY, not whether GitHub refused this commit. --- src/queue/processors.ts | 38 ++++++++++++++++++++ src/review/unified-comment-bridge.ts | 4 +++ src/review/unified-comment.ts | 23 ++++++++++++ test/unit/precision-breakers-chain.test.ts | 40 +++++++++++++++++++++ test/unit/unified-comment.test.ts | 41 ++++++++++++++++++++++ 5 files changed, 146 insertions(+) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 25db7015e3..e3d17236c7 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2506,6 +2506,16 @@ export function agentHoldAuditDetail(args: { ciState: string; ciHasPending: boolean; mergeableState: string | null | undefined; + /** #9862: the durable merge block the PLANNER already consults (activeMergeBlockedSha + the stored + * reason). GitHub can refuse a merge for reasons no gate models -- a stacked PR is unmergeable via the + * REST API at all (403), and a repo merge policy can refuse with a 405 -- so the executor records the + * refusal against that head and stops retrying. Without these here the resolver fell through to its + * residual "no merge action was planned", which is technically true and useless: it hides a condition + * that requires the MAINTAINER to act (merge in the web UI), and reads as the bot silently doing + * nothing. Only trusted when the block is for THIS head; a block from an older push is stale. */ + mergeBlockedSha?: string | null | undefined; + mergeBlockedReason?: string | null | undefined; + headSha?: string | null | undefined; approvalsSatisfied: boolean; authorIsOwner: boolean; authorIsAdmin: boolean; @@ -2546,6 +2556,12 @@ export function agentHoldAuditDetail(args: { return "merge withheld because required approvals are not satisfied"; if (args.mergeAutonomy !== "auto" && args.mergeAutonomy !== "auto_with_approval") return boundAgentHoldAuditReason(`merge withheld because merge autonomy is ${args.mergeAutonomy}`); + // A recorded refusal for THIS head is the most specific thing we know, and the only hold reason here + // that a maintainer must personally resolve. Checked last among the specifics but before the residual: + // everything above is a state LoopOver itself can still change, this one is not. + const blockedThisHead = + args.mergeBlockedReason && args.mergeBlockedSha && args.headSha && args.mergeBlockedSha === args.headSha; + if (blockedThisHead) return boundAgentHoldAuditReason(`merge withheld because GitHub refused the merge: ${args.mergeBlockedReason}`); return "merge withheld because no merge action was planned"; } if (args.gateBlockerCodes.length > 0) { @@ -3866,6 +3882,13 @@ async function runAgentMaintenancePlanAndExecute( ciState: ciAggregate.ciState, ciHasPending: ciAggregate.hasPending, mergeableState: liveMergeState ?? pr.mergeableState, + // The PASS's recorded instant (#9492), never a fresh Date.now(): a second clock read can disagree with + // the one the planner used inside a single pass, and replayDecision would then certify a decision made + // on an unrecorded read as "match" -- a false certification rather than a caught divergence. Same + // decisionClock the plan input above was built from, so planner and reporter cannot disagree. + mergeBlockedSha: activeMergeBlockedSha(pr, pr.headSha, decisionClock.nowMs), + mergeBlockedReason: pr.mergeBlockedReason, + headSha: pr.headSha, approvalsSatisfied, authorIsOwner, authorIsAdmin, @@ -12889,6 +12912,21 @@ async function maybePublishPrPublicSurface( // A preflight HOLD (e.g. the review lane is unavailable → the review is incomplete) must never render as // "safe to merge"; the renderer downgrades an otherwise-ready status to a manual-review hold. (#2002) preflightHeld: preflight.status === "hold", + // #9862: GitHub's own durable refusal for THIS head, so the comment can never claim "safe to merge" + // on a PR the executor has already been told it may not merge -- a stacked PR (403) or a repo merge + // policy (405) is invisible in mergeable_state, which reads perfectly `clean`. + // + // Matched on the head SHA alone, deliberately NOT through activeMergeBlockedSha: that helper resolves + // the retry-cooldown expiry, which needs the decision pass's recorded instant (#9492), and this is + // the PUBLISH path -- a reporting surface with no decision clock, reached from callers that have + // none either. Threading one here would either invent a second clock (the exact false-certification + // hazard #9492 forbids) or thread a decision concept through a path that makes no decisions. The + // distinction costs nothing that matters: `merge_blocked_until` bounds when LoopOver may RETRY, not + // whether GitHub refused this commit. The refusal is a fact about this SHA either way, and if a + // later retry succeeds the PR merges and this comment stops existing. + ...(pr.mergeBlockedReason && pr.mergeBlockedSha && pr.headSha && pr.mergeBlockedSha === pr.headSha + ? { mergeBlockedReason: pr.mergeBlockedReason } + : {}), extraCollapsibles: [...buildPublicSafeCollapsibles({ repo, pr, diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index aa143fc0ee..c9b20df44d 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -444,6 +444,9 @@ export type UnifiedCommentBridgeArgs = { /** Preflight is holding this PR (e.g. the review lane is unavailable) — an otherwise-ready comment then renders * "held", never "safe to merge". (#2002) */ preflightHeld?: boolean | undefined; + /** #9862: GitHub's own durable refusal for THIS head (stacked-PR 403, repo merge-policy 405). Resolved by + * the caller from the stored block, so this module stays clock-free like every other bridge input. */ + mergeBlockedReason?: string | undefined; /** Public freshness marker for the posted/updated review comment. Defaults to the current publish time. */ reviewedAt?: string | number | Date | undefined; /** Linked-issue satisfaction advisory (#1961/#3906): the resolved {status, rationale} the processor computed @@ -996,6 +999,7 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string ...(args.heldForReview ? { heldForReview: true } : {}), ...(args.neverClosed ? { neverClosed: true } : {}), ...(args.preflightHeld ? { preflightHeld: true } : {}), + ...(args.mergeBlockedReason ? { mergeBlockedReason: args.mergeBlockedReason } : {}), commentVerbosity: args.commentVerbosity, }, extraCollapsibles, diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index c9c7695fae..246dcb343e 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -310,6 +310,14 @@ export interface UnifiedCommentContext { /** The PR's author is the repo owner or a protected automation bot — the disposition NEVER auto-closes them, * so a gate "close" verdict renders as "held", not "Closed" (#8/#9). */ neverClosed?: boolean; + /** GitHub itself REFUSED the merge for this head, and the refusal is durable (#9862). Not a gate state and + * not visible in `mergeable_state` — a stacked PR is unmergeable via the REST API at all (403 "Merging + * stacked PRs via this API is not supported"), and a repo merge policy can refuse with a 405 — so the PR + * reads perfectly clean and green while the merge can never happen without a human. Rendering "safe to + * merge" there is the #4220 class in its purest form: the comment promises an action the bot has already + * been told it cannot take. The reason string is GitHub's own, surfaced verbatim so the maintainer knows + * what to do (merge in the web UI) instead of watching a green PR sit. */ + mergeBlockedReason?: string | undefined; /** Preflight is HOLDING this PR (e.g. the review lane is unavailable so the review is incomplete) — an * otherwise-ready status must then render as "held" (manual review), never "safe to merge". (#2002) */ preflightHeld?: boolean; @@ -401,6 +409,10 @@ export function deriveUnifiedStatus(input: UnifiedReviewInput, ctx: UnifiedComme // unfinished review. Downgrade it to a manual-review hold. Applied only to an otherwise-`ready` status, so it can // only ever DOWNGRADE, never approve. (#2002) — NOTE: a gate `merge` verdict WITH advisory blockers stays // authoritative-ready by design (the gate already weighed those); tightening THAT is the gate's confidence/bar. + // A durable GitHub merge refusal (#9862): same downgrade-only discipline as the guardrail hold above. The + // gate genuinely passed, so everything else about the review stands -- but the merge will not happen, and + // the status must not claim otherwise. + if (status === "ready" && ctx.mergeBlockedReason) return "held"; if (status === "ready" && ctx.preflightHeld) return "held"; // Held-vs-closed disposition parity (#8/#9): owner/automation-bot authors may be exempt from auto-close, so a // close verdict on those authors is rendered as held. Guardrail holds are handled above only for otherwise-ready @@ -480,6 +492,17 @@ function verdictLine(status: UnifiedCommentStatus, input: UnifiedReviewInput, ct case "advisory": return nestedBox(`**${icon} Suggested Action - Advisory Only**${reasons("no action taken")}`); case "held": + // #9862: when GitHub itself refused the merge, say so IN the verdict box. It is the one hold a + // maintainer must personally clear, and the previous behaviour ("Manual Review" with no reason, since + // a merge block sets no verdictReason) is what made a stacked PR look like the bot silently gave up. + // GitHub's own message is quoted verbatim -- it names the actual restriction and the remedy. + if (ctx.mergeBlockedReason) { + return nestedBox( + `**${icon} Suggested Action - Manual Review**\n` + + actionReasonBullets(`GitHub refused an automated merge for this commit: ${ctx.mergeBlockedReason}`) + + `\n${actionReasonBullets("The review passed — merge this pull request yourself to complete it.")}`, + ); + } return nestedBox(`**${icon} Suggested Action - Manual Review**${reasons()}`); case "blocked": if (ctx.neverClosed) { diff --git a/test/unit/precision-breakers-chain.test.ts b/test/unit/precision-breakers-chain.test.ts index d15dfe5e6f..a69180d178 100644 --- a/test/unit/precision-breakers-chain.test.ts +++ b/test/unit/precision-breakers-chain.test.ts @@ -246,3 +246,43 @@ describe("agentHoldAuditDetail — durable why-no-action audit reason", () => { expect(agentHoldAuditDetail({ ...base, gateConclusion: "neutral" })).toBe("no auto-action planned"); }); }); + +describe("agentHoldAuditDetail surfaces a GitHub merge refusal (#9862)", () => { + // JSONbored/loopover#9856: gate success, CI passed, mergeable clean, autonomy auto, approvals satisfied — + // and still held, because GitHub had refused the merge with a 403 (stacked PRs are unmergeable via the REST + // API). The resolver fell through to its residual "no merge action was planned", which is true and useless. + const STACKED_403 = "merge forbidden (403): Merging stacked PRs via this API is not supported. Use the web interface instead."; + const green = { + planned: [] as never[], breakerOnPlan: [] as never[], precisionBreakerEngaged: false, closeAuditHoldoutEngaged: false, + gateConclusion: "success", gateBlockerCodes: [] as string[], ciState: "passed", ciHasPending: false, + mergeableState: "clean", approvalsSatisfied: true, authorIsOwner: false, authorIsAdmin: false, + authorIsAutomationBot: false, closeOwnerAuthors: false, mergeAutonomy: "auto", closeAutonomy: "auto", + }; + + it("REGRESSION: names GitHub's refusal instead of the residual", () => { + const detail = agentHoldAuditDetail({ ...green, headSha: "abc123", mergeBlockedSha: "abc123", mergeBlockedReason: STACKED_403 }); + expect(detail).toContain("GitHub refused the merge"); + expect(detail).toContain("stacked PRs via this API is not supported"); + expect(detail).not.toContain("no merge action was planned"); + }); + + it("INVARIANT: a block recorded against a DIFFERENT head is stale and must not be reported", () => { + // The contributor pushed since the refusal; that new head has never been attempted, so claiming GitHub + // refused it would be a lie — and would hide whatever is actually holding the new commit. + const detail = agentHoldAuditDetail({ ...green, headSha: "newsha", mergeBlockedSha: "oldsha", mergeBlockedReason: STACKED_403 }); + expect(detail).toBe("merge withheld because no merge action was planned"); + }); + + it("INVARIANT: more specific gate/CI/mergeable reasons still win over the block", () => { + // The block is checked last among the specifics: everything above it is a state LoopOver can still + // change on its own, so reporting the refusal there would misdirect the reader. + const blocked = { headSha: "abc", mergeBlockedSha: "abc", mergeBlockedReason: STACKED_403 }; + expect(agentHoldAuditDetail({ ...green, ...blocked, ciHasPending: true })).toBe("auto-action held because CI is still pending"); + expect(agentHoldAuditDetail({ ...green, ...blocked, mergeableState: "dirty" })).toContain("conflicts with the base branch"); + expect(agentHoldAuditDetail({ ...green, ...blocked, approvalsSatisfied: false })).toContain("required approvals are not satisfied"); + }); + + it("INVARIANT: no block ⇒ the residual is unchanged", () => { + expect(agentHoldAuditDetail({ ...green, headSha: "abc" })).toBe("merge withheld because no merge action was planned"); + }); +}); diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index cf271cc16f..6ab4429aaa 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -1059,3 +1059,44 @@ describe("shouldPostReviewingPlaceholder", () => { expect(shouldPostReviewingPlaceholder({ reviewWillRun: true, mode: "live", willComment: false })).toBe(false); }); }); + +describe("a GitHub merge refusal must never render as safe to merge (#9862)", () => { + // Observed live on JSONbored/loopover#9856: the panel said "Suggested Action - Approve/Merge / safe to + // merge" and the bot posted "LoopOver approves — the gate is satisfied and CI is green", while GitHub had + // already refused the merge with a 403 ("Merging stacked PRs via this API is not supported"). Everything + // the gate measures WAS green; mergeable_state read `clean`; the refusal is invisible to all of it. The + // maintainer saw a green, approved PR sit unmerged with no stated reason — repeatedly, across weeks. + const STACKED_403 = "merge forbidden (403): Merging stacked PRs via this API is not supported. Use the web interface instead."; + const blockedBase = { decision: "merge" as const, readiness: { ciState: "passed" as const } }; + + it("REGRESSION: an otherwise-ready PR with a merge block renders HELD, not ready", () => { + expect(deriveUnifiedStatus(blockedBase as never, { mergeBlockedReason: STACKED_403 })).toBe("held"); + }); + + it("INVARIANT: downgrade-only — it never upgrades a blocked or advisory status", () => { + // Same discipline as the guardrail hold: a real gate/CI block above must still win, and an advisory + // review must not become a hold just because a stale block string is present. + expect(deriveUnifiedStatus({ ...blockedBase, readiness: { ciState: "failed" } } as never, { mergeBlockedReason: STACKED_403 })).not.toBe("held"); + expect(deriveUnifiedStatus({ ...blockedBase, decision: "advisory" } as never, { mergeBlockedReason: STACKED_403 })).toBe("advisory"); + }); + + it("INVARIANT: no block ⇒ unchanged, so every unblocked PR reads exactly as before", () => { + expect(deriveUnifiedStatus(blockedBase as never, {})).toBe("ready"); + expect(deriveUnifiedStatus(blockedBase as never, { mergeBlockedReason: undefined })).toBe("ready"); + }); + + it("the rendered comment NAMES GitHub's refusal and tells the maintainer what to do", () => { + // Uses the file's own full fixture: the renderer needs a complete input, and the point of this test is + // the verdict box's content, not the minimal-shape handling the status tests above already cover. + const body = renderUnifiedReviewComment( + { ...base, summary: "The change is sound.", decision: "merge", readiness: { ciState: "passed" }, reviewerCount: 1 } as never, + { mergeBlockedReason: STACKED_403 } as never, + ); + expect(body).toContain("Manual Review"); + expect(body).toContain("GitHub refused an automated merge"); + expect(body).toContain("stacked PRs via this API is not supported"); + expect(body).toContain("merge this pull request yourself"); + // And it must NOT still be claiming the thing that cannot happen. + expect(body).not.toContain("safe to merge"); + }); +});