diff --git a/apps/server/src/sourceControl/GitHubCli.test.ts b/apps/server/src/sourceControl/GitHubCli.test.ts index 4b989bbbacba..c4ec6fd00f6a 100644 --- a/apps/server/src/sourceControl/GitHubCli.test.ts +++ b/apps/server/src/sourceControl/GitHubCli.test.ts @@ -1,7 +1,9 @@ import { assert, it, afterEach, describe, expect, vi } from "@effect/vitest"; import * as Effect from "effect/Effect"; +import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; import * as PlatformError from "effect/PlatformError"; +import * as TestClock from "effect/testing/TestClock"; import { ChildProcessSpawner } from "effect/unstable/process"; import { VcsProcessExitError, VcsProcessSpawnError } from "@t3tools/contracts"; @@ -389,6 +391,14 @@ describe("GitHubCli.layer", () => { ), ), ); + mockRun.mockReturnValueOnce( + Effect.succeed( + processOutput( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ mergeable: "MERGEABLE", mergeStateStatus: "CLEAN" }), + ), + ), + ); mockRun.mockReturnValueOnce(Effect.succeed(processOutput(""))); const gh = yield* GitHubCli.GitHubCli; @@ -407,6 +417,13 @@ describe("GitHubCli.layer", () => { timeoutMs: 30_000, }); expect(mockRun).toHaveBeenNthCalledWith(2, { + operation: "GitHubCli.execute", + command: "gh", + args: ["pr", "view", "#42", "--json", "mergeable,mergeStateStatus"], + cwd: "/repo", + timeoutMs: 30_000, + }); + expect(mockRun).toHaveBeenNthCalledWith(3, { operation: "GitHubCli.execute", command: "gh", args: ["pr", "merge", "#42", "--merge"], @@ -430,12 +447,20 @@ describe("GitHubCli.layer", () => { ), ), ); + mockRun.mockReturnValueOnce( + Effect.succeed( + processOutput( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ mergeable: "MERGEABLE", mergeStateStatus: "CLEAN" }), + ), + ), + ); mockRun.mockReturnValueOnce(Effect.succeed(processOutput(""))); const gh = yield* GitHubCli.GitHubCli; yield* gh.mergePullRequest({ cwd: "/repo", reference: "#42" }); - expect(mockRun).toHaveBeenNthCalledWith(2, { + expect(mockRun).toHaveBeenNthCalledWith(3, { operation: "GitHubCli.execute", command: "gh", args: ["pr", "merge", "#42", "--squash"], @@ -459,12 +484,20 @@ describe("GitHubCli.layer", () => { ), ), ); + mockRun.mockReturnValueOnce( + Effect.succeed( + processOutput( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ mergeable: "MERGEABLE", mergeStateStatus: "CLEAN" }), + ), + ), + ); mockRun.mockReturnValueOnce(Effect.succeed(processOutput(""))); const gh = yield* GitHubCli.GitHubCli; yield* gh.mergePullRequest({ cwd: "/repo", reference: "#42" }); - expect(mockRun).toHaveBeenNthCalledWith(2, { + expect(mockRun).toHaveBeenNthCalledWith(3, { operation: "GitHubCli.execute", command: "gh", args: ["pr", "merge", "#42", "--rebase"], @@ -477,12 +510,147 @@ describe("GitHubCli.layer", () => { it.effect("falls back to a plain merge when repository settings can't be read", () => Effect.gen(function* () { mockRun.mockReturnValueOnce(Effect.succeed(processOutput("not json"))); + mockRun.mockReturnValueOnce( + Effect.succeed( + processOutput( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ mergeable: "MERGEABLE", mergeStateStatus: "CLEAN" }), + ), + ), + ); mockRun.mockReturnValueOnce(Effect.succeed(processOutput(""))); const gh = yield* GitHubCli.GitHubCli; yield* gh.mergePullRequest({ cwd: "/repo", reference: "#42" }); + expect(mockRun).toHaveBeenNthCalledWith(3, { + operation: "GitHubCli.execute", + command: "gh", + args: ["pr", "merge", "#42", "--merge"], + cwd: "/repo", + timeoutMs: 30_000, + }); + }).pipe(Effect.provide(layer)), + ); + + it.effect("polls until GitHub finishes computing mergeability before merging", () => + Effect.gen(function* () { + mockRun.mockReturnValueOnce( + Effect.succeed( + processOutput( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + mergeCommitAllowed: true, + squashMergeAllowed: true, + rebaseMergeAllowed: true, + }), + ), + ), + ); + // First read: still computing. Second read: settled. + mockRun.mockReturnValueOnce( + Effect.succeed( + processOutput( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ mergeable: "UNKNOWN", mergeStateStatus: "UNKNOWN" }), + ), + ), + ); + mockRun.mockReturnValueOnce( + Effect.succeed( + processOutput( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ mergeable: "MERGEABLE", mergeStateStatus: "CLEAN" }), + ), + ), + ); + mockRun.mockReturnValueOnce(Effect.succeed(processOutput(""))); + + const gh = yield* GitHubCli.GitHubCli; + const merge = yield* gh + .mergePullRequest({ cwd: "/repo", reference: "#42" }) + .pipe(Effect.forkChild({ startImmediately: true })); + // Let the one-second poll backoff elapse so the second read runs. + yield* TestClock.adjust("1 second"); + yield* Fiber.join(merge); + expect(mockRun).toHaveBeenNthCalledWith(2, { + operation: "GitHubCli.execute", + command: "gh", + args: ["pr", "view", "#42", "--json", "mergeable,mergeStateStatus"], + cwd: "/repo", + timeoutMs: 30_000, + }); + expect(mockRun).toHaveBeenNthCalledWith(3, { + operation: "GitHubCli.execute", + command: "gh", + args: ["pr", "view", "#42", "--json", "mergeable,mergeStateStatus"], + cwd: "/repo", + timeoutMs: 30_000, + }); + expect(mockRun).toHaveBeenNthCalledWith(4, { + operation: "GitHubCli.execute", + command: "gh", + args: ["pr", "merge", "#42", "--merge"], + cwd: "/repo", + timeoutMs: 30_000, + }); + }).pipe(Effect.provide(layer)), + ); + + it.effect("retries a transient merge-blocked failure", () => + Effect.gen(function* () { + mockRun.mockReturnValueOnce( + Effect.succeed( + processOutput( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + mergeCommitAllowed: true, + squashMergeAllowed: true, + rebaseMergeAllowed: true, + }), + ), + ), + ); + mockRun.mockReturnValueOnce( + Effect.succeed( + processOutput( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ mergeable: "MERGEABLE", mergeStateStatus: "CLEAN" }), + ), + ), + ); + // First merge attempt: transient rejection. Second: success. + mockRun.mockReturnValueOnce( + Effect.fail( + new VcsProcessExitError({ + operation: "GitHubCli.execute", + command: "gh", + cwd: "/repo", + exitCode: 1, + detail: "blocked", + failureKind: "merge-blocked", + }), + ), + ); + mockRun.mockReturnValueOnce(Effect.succeed(processOutput(""))); + + const gh = yield* GitHubCli.GitHubCli; + const merge = yield* gh + .mergePullRequest({ cwd: "/repo", reference: "#42" }) + .pipe(Effect.forkChild({ startImmediately: true })); + // Let the one-second retry backoff elapse so the second attempt runs. + yield* TestClock.adjust("1 second"); + yield* Fiber.join(merge); + + expect(mockRun).toHaveBeenNthCalledWith(3, { + operation: "GitHubCli.execute", + command: "gh", + args: ["pr", "merge", "#42", "--merge"], + cwd: "/repo", + timeoutMs: 30_000, + }); + expect(mockRun).toHaveBeenNthCalledWith(4, { operation: "GitHubCli.execute", command: "gh", args: ["pr", "merge", "#42", "--merge"], diff --git a/apps/server/src/sourceControl/GitHubCli.ts b/apps/server/src/sourceControl/GitHubCli.ts index 8165a892aa23..00c8e59479d3 100644 --- a/apps/server/src/sourceControl/GitHubCli.ts +++ b/apps/server/src/sourceControl/GitHubCli.ts @@ -1,4 +1,5 @@ import * as Context from "effect/Context"; +import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; import * as Option from "effect/Option"; @@ -128,7 +129,7 @@ export class GitHubMergeBlockedError extends Schema.TaggedErrorClass + execute({ + cwd, + args: ["repo", "view", "--json", "mergeCommitAllowed,squashMergeAllowed,rebaseMergeAllowed"], + }).pipe( + Effect.flatMap((output) => + decodeRawGitHubMergeMethods(output.stdout).pipe( + // If the settings can't be read, fall back to a plain merge commit + // and let GitHub surface the real reason if that method is refused. + Effect.orElseSucceed(() => ({ + mergeCommitAllowed: true, + squashMergeAllowed: false, + rebaseMergeAllowed: false, + })), + ), + ), + Effect.map(mergeMethodFlag), + ); + + const awaitMergeabilityComputed = (cwd: string, reference: string) => + Effect.gen(function* () { + for (let attempt = 0; attempt < MERGEABILITY_POLL_ATTEMPTS; attempt++) { + const state = yield* execute({ + cwd, + args: ["pr", "view", reference, "--json", "mergeable,mergeStateStatus"], + }).pipe( + Effect.flatMap((output) => + decodeRawGitHubMergeability(output.stdout).pipe( + Effect.orElseSucceed(() => UNKNOWN_MERGEABILITY), + ), + ), + // A failed read must not abort the merge; fall through and let the + // merge itself report any real problem. + Effect.orElseSucceed(() => UNKNOWN_MERGEABILITY), + ); + if (state.mergeable !== "UNKNOWN") { + return; + } + if (attempt < MERGEABILITY_POLL_ATTEMPTS - 1) { + yield* Effect.sleep(MERGEABILITY_POLL_INTERVAL); + } + } + }); + + const attemptMerge = ( + cwd: string, + args: ReadonlyArray, + attemptsLeft: number, + ): Effect.Effect => + execute({ cwd, args }).pipe( + Effect.asVoid, + Effect.catchTag("GitHubMergeBlockedError", (error) => + attemptsLeft <= 1 + ? Effect.fail(error) + : Effect.sleep(MERGE_RETRY_INTERVAL).pipe( + Effect.flatMap(() => attemptMerge(cwd, args, attemptsLeft - 1)), + ), + ), + ); + return GitHubCli.of({ execute, listOpenPullRequests: (input) => @@ -585,34 +678,18 @@ export const make = Effect.gen(function* () { args: ["pr", "checkout", input.reference, ...(input.force ? ["--force"] : [])], }).pipe(Effect.asVoid), mergePullRequest: (input) => - execute({ - cwd: input.cwd, - args: [ - "repo", - "view", - "--json", - "mergeCommitAllowed,squashMergeAllowed,rebaseMergeAllowed", - ], - }).pipe( - Effect.flatMap((output) => - decodeRawGitHubMergeMethods(output.stdout).pipe( - // If the settings can't be read, fall back to a plain merge commit - // and let GitHub surface the real reason if that method is refused. - Effect.orElseSucceed(() => ({ - mergeCommitAllowed: true, - squashMergeAllowed: false, - rebaseMergeAllowed: false, - })), - ), - ), - Effect.map(mergeMethodFlag), + resolveMergeMethodFlag(input.cwd).pipe( + // Force GitHub to finish computing mergeability before we merge, so a + // merge fired right after create/push doesn't race a transient + // "not mergeable" rejection. + Effect.tap(() => awaitMergeabilityComputed(input.cwd, input.reference)), Effect.flatMap((flag) => - execute({ - cwd: input.cwd, - args: ["pr", "merge", input.reference, ...(flag ? [flag] : ["--merge"])], - }), + attemptMerge( + input.cwd, + ["pr", "merge", input.reference, ...(flag ? [flag] : ["--merge"])], + MERGE_ATTEMPTS, + ), ), - Effect.asVoid, ), }); });