diff --git a/.changeset/fn-8835-pr-merge-readiness.md b/.changeset/fn-8835-pr-merge-readiness.md new file mode 100644 index 0000000000..c876e83bbb --- /dev/null +++ b/.changeset/fn-8835-pr-merge-readiness.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Prevent auto-merge attempts for branch-protected, behind, conflicting, or unknown PR states. +category: fix +dev: The legacy PR merge gate now requires normalized mergeability to be `clean` while preserving optional approval and check policy. diff --git a/packages/cli/src/commands/__tests__/task-lifecycle.test.ts b/packages/cli/src/commands/__tests__/task-lifecycle.test.ts index 84b364c083..aef8266ed8 100644 --- a/packages/cli/src/commands/__tests__/task-lifecycle.test.ts +++ b/packages/cli/src/commands/__tests__/task-lifecycle.test.ts @@ -517,7 +517,7 @@ describe("processPullRequestMergeTask", () => { expect(github.createPr).toHaveBeenCalledWith(expect.objectContaining({ head: getTaskBranchName(task.id) })); }); - it("does not create duplicate group PR when branch-group PR already exists", async () => { + it("holds a branch-protected shared-group PR without calling mergePr", async () => { const task: MockTask = { id: "FN-9012", title: "group member", @@ -553,11 +553,11 @@ describe("processPullRequestMergeTask", () => { findPrForBranch: vi.fn(async () => null), createPr: vi.fn(), getPrMergeStatus: vi.fn(async () => ({ - prInfo: { number: 22, status: "open" as const, url: "https://github.com/x/y/pull/22" }, - reviewDecision: null, + prInfo: { number: 22, status: "open" as const, url: "https://github.com/x/y/pull/22", mergeable: "blocked" as const }, + reviewDecision: "REVIEW_REQUIRED", checks: [], mergeReady: false, - blockingReasons: [], + blockingReasons: ["PR mergeability is blocked"], })), mergePr: vi.fn(), }; @@ -566,6 +566,8 @@ describe("processPullRequestMergeTask", () => { expect(github.createPr).not.toHaveBeenCalled(); expect(github.getPrMergeStatus).toHaveBeenCalledWith("owner", "repo", 22); + expect(github.mergePr).not.toHaveBeenCalled(); + expect(store.updateTask).toHaveBeenCalledWith(task.id, { status: "awaiting-pr-checks" }); expect(store.updateBranchGroup).toHaveBeenCalledWith("BG-2", expect.objectContaining({ prNumber: 22, prUrl: "https://github.com/x/y/pull/22", @@ -845,7 +847,7 @@ describe("processPullRequestMergeTask", () => { }); }); - it("does not bump mergeRetries for a non-conflicting not-ready PR", async () => { + it("holds a branch-protected review-required PR without a merge attempt or conflict retry", async () => { const task: MockTask = { id: "FN-9021", title: "test", @@ -862,11 +864,11 @@ describe("processPullRequestMergeTask", () => { findPrForBranch: vi.fn(async () => existingPr), createPr: vi.fn(), getPrMergeStatus: vi.fn(async () => ({ - prInfo: { ...existingPr, mergeable: "unknown" as const }, - reviewDecision: null, + prInfo: { ...existingPr, mergeable: "blocked" as const }, + reviewDecision: "REVIEW_REQUIRED", checks: [], mergeReady: false, - blockingReasons: [], + blockingReasons: ["PR mergeability is blocked"], })), mergePr: vi.fn(), }; @@ -875,6 +877,7 @@ describe("processPullRequestMergeTask", () => { expect(result).toBe("waiting"); expect(store.updateTask).toHaveBeenCalledWith(task.id, { status: "awaiting-pr-checks" }); + expect(github.mergePr).not.toHaveBeenCalled(); }); it("skips the push when an existing PR already covers the branch", async () => { @@ -1456,7 +1459,7 @@ describe("processPullRequestMergeTask", () => { // checks and no blocking review state, so isPrMergeReady returns // mergeReady: true. Without the gate this would auto-merge. return { - prInfo, + prInfo: { ...prInfo, mergeable: "clean" as const }, reviewDecision, checks: [], mergeReady: true, diff --git a/packages/dashboard/src/__tests__/github.test.ts b/packages/dashboard/src/__tests__/github.test.ts index 13be256bc9..7f1b5a55d2 100644 --- a/packages/dashboard/src/__tests__/github.test.ts +++ b/packages/dashboard/src/__tests__/github.test.ts @@ -1665,6 +1665,8 @@ describe("GitHubClient", () => { const result = await client.getPrMergeStatus("owner", "repo", 42); expect(result.mergeable).toBe("conflicting"); expect(result.prInfo.mergeable).toBe("conflicting"); + expect(result.mergeReady).toBe(false); + expect(result.blockingReasons).toEqual(["PR mergeability is conflicting"]); }); it("maps BEHIND merge-state to behind", async () => { @@ -1684,6 +1686,8 @@ describe("GitHubClient", () => { const result = await client.getPrMergeStatus("owner", "repo", 42); expect(result.mergeable).toBe("behind"); expect(result.prInfo.mergeable).toBe("behind"); + expect(result.mergeReady).toBe(false); + expect(result.blockingReasons).toEqual(["PR mergeability is behind"]); }); it("maps BLOCKED merge-state to blocked", async () => { @@ -1693,7 +1697,8 @@ describe("GitHubClient", () => { url: "https://github.com/owner/repo/pull/42", title: "Blocked PR", state: "OPEN", - reviewDecision: "APPROVED", + reviewDecision: "REVIEW_REQUIRED", + mergeable: "MERGEABLE", mergeStateStatus: "BLOCKED", baseRefName: "main", headRefName: "fusion/fn-093", @@ -1703,6 +1708,8 @@ describe("GitHubClient", () => { const result = await client.getPrMergeStatus("owner", "repo", 42); expect(result.mergeable).toBe("blocked"); expect(result.prInfo.mergeable).toBe("blocked"); + expect(result.mergeReady).toBe(false); + expect(result.blockingReasons).toEqual(["PR mergeability is blocked"]); }); it("maps missing mergeability fields to unknown", async () => { @@ -1721,9 +1728,11 @@ describe("GitHubClient", () => { const result = await client.getPrMergeStatus("owner", "repo", 42); expect(result.mergeable).toBe("unknown"); expect(result.prInfo.mergeable).toBe("unknown"); + expect(result.mergeReady).toBe(false); + expect(result.blockingReasons).toEqual(["PR mergeability is unknown"]); }); - it("falls back to GraphQL API when gh CLI merge-status lookup fails and token is available", async () => { + it("fails closed through the GraphQL API when branch protection blocks a review-required PR", async () => { mockRunGhJsonAsync.mockRejectedValue(new Error("gh failed")); const clientWithToken = new GitHubClient("ghp_token"); const mockFetch = vi.fn().mockResolvedValue({ @@ -1734,11 +1743,11 @@ describe("GitHubClient", () => { pullRequest: { number: 42, url: "https://github.com/owner/repo/pull/42", - title: "Fallback PR", + title: "Branch-protected PR", state: "OPEN", - reviewDecision: null, - mergeable: "CONFLICTING", - mergeStateStatus: "DIRTY", + reviewDecision: "REVIEW_REQUIRED", + mergeable: "MERGEABLE", + mergeStateStatus: "BLOCKED", baseRefName: "main", headRefName: "fusion/fn-093", comments: { totalCount: 0 }, @@ -1790,9 +1799,10 @@ describe("GitHubClient", () => { const result = await clientWithToken.getPrMergeStatus("owner", "repo", 42); - expect(result.mergeReady).toBe(true); - expect(result.mergeable).toBe("conflicting"); - expect(result.prInfo.mergeable).toBe("conflicting"); + expect(result.mergeReady).toBe(false); + expect(result.mergeable).toBe("blocked"); + expect(result.prInfo.mergeable).toBe("blocked"); + expect(result.blockingReasons).toEqual(["PR mergeability is blocked"]); expect(result.checks).toEqual([ { name: "ci", @@ -2140,7 +2150,7 @@ describe("GitHubClient", () => { describe("isPrMergeReady", () => { it("blocks closed PRs", () => { - expect(isPrMergeReady({ status: "closed", reviewDecision: null, checks: [] })).toEqual({ + expect(isPrMergeReady({ status: "closed", reviewDecision: null, checks: [], mergeable: "clean" })).toEqual({ ready: false, blockingReasons: ["PR is closed"], }); @@ -2151,6 +2161,7 @@ describe("GitHubClient", () => { status: "open", reviewDecision: "CHANGES_REQUESTED", checks: [{ name: "ci", required: true, state: "success" }], + mergeable: "clean", })).toEqual({ ready: false, blockingReasons: ["changes requested review is active"], @@ -2162,6 +2173,7 @@ describe("GitHubClient", () => { status: "open", reviewDecision: null, checks: [{ name: "ci", required: true, state: "pending" }], + mergeable: "clean", })).toEqual({ ready: false, blockingReasons: ["required checks not successful: ci (pending)"], @@ -2176,8 +2188,40 @@ describe("GitHubClient", () => { { name: "required-ci", required: true, state: "success" }, { name: "optional-preview", required: false, state: "failure" }, ], + mergeable: "clean", })).toEqual({ ready: true, blockingReasons: [] }); }); + + it.each(["blocked", "behind", "conflicting", "unknown"] as const)( + "fails closed when mergeability is %s", + (mergeable) => { + expect(isPrMergeReady({ + status: "open", + reviewDecision: "APPROVED", + checks: [{ name: "ci", required: true, state: "success" }], + mergeable, + })).toEqual({ + ready: false, + blockingReasons: [`PR mergeability is ${mergeable}`], + }); + }, + ); + + it("aggregates branch protection with existing review and required-check blockers", () => { + expect(isPrMergeReady({ + status: "open", + reviewDecision: "CHANGES_REQUESTED", + checks: [{ name: "ci", required: true, state: "failure" }], + mergeable: "blocked", + })).toEqual({ + ready: false, + blockingReasons: [ + "changes requested review is active", + "PR mergeability is blocked", + "required checks not successful: ci (failure)", + ], + }); + }); }); describe("error handling when gh CLI not available", () => { diff --git a/packages/dashboard/src/github.ts b/packages/dashboard/src/github.ts index 6bb0e6d10f..954ed0d8c2 100644 --- a/packages/dashboard/src/github.ts +++ b/packages/dashboard/src/github.ts @@ -735,10 +735,15 @@ function toPrInfo(input: { }; } +/* +FNXC:PrMergeReadiness 2026-08-09-00:48: +FN-8835 requires the live pull-request merge gate to fail closed on GitHub's normalized mergeability state. Branch-protection BLOCKED, stale BEHIND, conflicts, and unknown state must wait before any merge request; legacy approval and optional-check policy remain unchanged. +*/ export function isPrMergeReady(input: { status: PrInfo["status"]; reviewDecision: ReviewDecision; checks: PrCheckStatus[]; + mergeable: PrConflictState; }): { ready: boolean; blockingReasons: string[] } { const blockingReasons: string[] = []; @@ -750,6 +755,10 @@ export function isPrMergeReady(input: { blockingReasons.push("changes requested review is active"); } + if (input.mergeable !== "clean") { + blockingReasons.push(`PR mergeability is ${input.mergeable}`); + } + const blockingChecks = input.checks.filter( (check) => check.required && check.state !== "success", ); @@ -1816,6 +1825,7 @@ export class GitHubClient { status: prInfo.status, reviewDecision: pr.reviewDecision ?? null, checks: normalizedChecks, + mergeable, }); return { @@ -1975,6 +1985,7 @@ export class GitHubClient { status: prInfo.status, reviewDecision: pr.reviewDecision, checks, + mergeable, }); return {