Skip to content

fix(server): stop merge button racing GitHub mergeability computation #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
enisze merged 1 commit into from
Aug 1, 2026
Merged

fix(server): stop merge button racing GitHub mergeability computation #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
172 changes: 170 additions & 2 deletions apps/server/src/sourceControl/GitHubCli.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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";

Expand Down Expand Up @@ -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;
Expand All @@ -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"],
Expand All @@ -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"],
Expand All @@ -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"],
Expand All @@ -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"],
Expand Down
131 changes: 104 additions & 27 deletions apps/server/src/sourceControl/GitHubCli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -128,7 +129,7 @@ export class GitHubMergeBlockedError extends Schema.TaggedErrorClass<GitHubMerge
gitHubCliFailureFields,
) {
get detail(): string {
return "GitHub wouldn't merge this pull request. It may have conflicts, failing required checks, branch protection, or the repository may not allow the merge-commit method.";
return "GitHub wouldn't merge this pull request. It may have merge conflicts, failing required status checks, or branch protection rules that block the merge.";
}

override get message(): string {
Expand Down Expand Up @@ -335,6 +336,38 @@ const decodeRawGitHubMergeMethods = Schema.decodeEffect(
Schema.fromJsonString(RawGitHubMergeMethodsSchema),
);

const RawGitHubMergeabilitySchema = Schema.Struct({
mergeable: Schema.String,
mergeStateStatus: Schema.String,
});
const decodeRawGitHubMergeability = Schema.decodeEffect(
Schema.fromJsonString(RawGitHubMergeabilitySchema),
);
const UNKNOWN_MERGEABILITY = {
mergeable: "UNKNOWN",
mergeStateStatus: "UNKNOWN",
} as const;

/**
* Right after a pull request is opened (or its branch is pushed), GitHub reports
* the mergeable state as `UNKNOWN` while it recomputes the merge in the
* background. A merge attempted inside that window is rejected with a transient
* "not mergeable" / "base branch was modified" error that our classifier can
* only see as a hard `merge-blocked` failure. Reading the state forces GitHub to
* compute it, so we poll (bounded) until it settles before merging.
*/
const MERGEABILITY_POLL_ATTEMPTS = 6;
const MERGEABILITY_POLL_INTERVAL = Duration.seconds(1);

/**
* Even once mergeability is computed, GitHub can briefly return a transient
* `merge-blocked` failure. Retry the merge itself a few times; a genuine
* conflict, permission problem, or protection rule won't clear on retry, so we
* only retry that one classified kind.
*/
const MERGE_ATTEMPTS = 3;
const MERGE_RETRY_INTERVAL = Duration.seconds(1);

/**
* `gh pr merge` requires an explicit merge method; without one it drops into an
* interactive prompt that has no TTY here and fails. Hardcoding `--merge` breaks
Expand Down Expand Up @@ -451,6 +484,66 @@ export const make = Effect.gen(function* () {
}),
);

const resolveMergeMethodFlag = (cwd: string) =>
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<string>,
attemptsLeft: number,
): Effect.Effect<void, GitHubCliError> =>
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) =>
Expand Down Expand Up @@ -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,
),
});
});
Expand Down