Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions src/review/unified-comment-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
23 changes: 23 additions & 0 deletions src/review/unified-comment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
40 changes: 40 additions & 0 deletions test/unit/precision-breakers-chain.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
});
41 changes: 41 additions & 0 deletions test/unit/unified-comment.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
});