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"); + }); +});