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
89 changes: 66 additions & 23 deletions electron/host-service/scm-runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ import type {
GraphResult,
} from "../../src/lib/git-graph/types";
import { parseWorktreePathByBranch } from "../../src/lib/source-control-worktrees";
import type { PrMergeMethod } from "../../src/lib/pr-status";
import type {
ConcretePrMergeMethod,
PrMergeMethod,
} from "../../src/lib/pr-status";
import type { DetachedCheckoutResult } from "../main/types";
import {
buildSourceControlDiffPreview,
Expand Down Expand Up @@ -1628,15 +1631,13 @@ export async function mergeScmPr(args: {
if (!authResult.ok) {
return { ok: false, stderr: "GitHub CLI is not authenticated." };
}
const method = args.method ?? "default";
const method = await resolveScmMergeMethod({
method: args.method,
cwd: args.cwd,
});
const result = await runCommandArgs({
command: "gh",
commandArgs: [
"pr",
"merge",
...(method === "default" ? [] : [`--${method}`]),
"--delete-branch",
],
commandArgs: buildMergePullRequestArgs(method),
cwd: args.cwd,
});
invalidateCachedGhAuthOnFailure(result, args.cwd);
Expand Down Expand Up @@ -1707,15 +1708,15 @@ export function buildCreatePullRequestArgs(args: {
}

export function buildAutoMergePullRequestArgs(
method: PrMergeMethod = "default",
method: ConcretePrMergeMethod = "squash",
) {
return [
"pr",
"merge",
"--auto",
...(method === "default" ? [] : [`--${method}`]),
"--delete-branch",
];
return ["pr", "merge", "--auto", `--${method}`, "--delete-branch"];
}

export function buildMergePullRequestArgs(
method: ConcretePrMergeMethod = "squash",
) {
return ["pr", "merge", `--${method}`, "--delete-branch"];
}

export function classifyAutoMergeFailure(stderr: string) {
Expand Down Expand Up @@ -1798,6 +1799,49 @@ export async function fetchRepoMergeSettings(args: { cwd?: string }) {
}
}

const MERGE_METHOD_PREFERENCE = ["squash", "merge", "rebase"] as const;

/**
* `gh pr merge` refuses to run without an explicit strategy flag when it is not
* attached to a TTY, so "default" has to be resolved to a concrete strategy
* before the command is spawned. The repository's allowed merge methods decide
* which one wins; when they cannot be read we fall back to squash.
*/
export function pickAllowedMergeMethod(args: {
method?: PrMergeMethod;
settings?: {
squashMergeAllowed?: boolean;
mergeCommitAllowed?: boolean;
rebaseMergeAllowed?: boolean;
};
}): ConcretePrMergeMethod {
if (args.method && args.method !== "default") {
return args.method;
}
const allowed: Record<ConcretePrMergeMethod, boolean> = {
squash: args.settings?.squashMergeAllowed ?? true,
merge: args.settings?.mergeCommitAllowed ?? true,
rebase: args.settings?.rebaseMergeAllowed ?? true,
};
return MERGE_METHOD_PREFERENCE.find((method) => allowed[method]) ?? "squash";
}

async function resolveScmMergeMethod(args: {
method?: PrMergeMethod;
cwd?: string;
}): Promise<ConcretePrMergeMethod> {
if (args.method && args.method !== "default") {
return args.method;
}
const settings = await fetchRepoMergeSettings({ cwd: args.cwd }).catch(
() => undefined,
);
return pickAllowedMergeMethod({
method: args.method,
settings: settings?.ok ? settings : undefined,
});
}

export async function createScmPullRequest(args: {
title: string;
body?: string;
Expand Down Expand Up @@ -1857,9 +1901,13 @@ export async function createScmPullRequest(args: {
return { ok: true, prUrl, autoMergeEnabled: false, stderr: "" };
}

const resolvedMergeMethod = await resolveScmMergeMethod({
method: mergeMethod,
cwd,
});
const autoMergeResult = await runCommandArgs({
command: "gh",
commandArgs: buildAutoMergePullRequestArgs(mergeMethod),
commandArgs: buildAutoMergePullRequestArgs(resolvedMergeMethod),
cwd,
});
invalidateCachedGhAuthOnFailure(autoMergeResult, cwd);
Expand All @@ -1870,12 +1918,7 @@ export async function createScmPullRequest(args: {
if (failure === "clean-status") {
const mergeResult = await runCommandArgs({
command: "gh",
commandArgs: [
"pr",
"merge",
...(mergeMethod === "default" ? [] : [`--${mergeMethod}`]),
"--delete-branch",
],
commandArgs: buildMergePullRequestArgs(resolvedMergeMethod),
cwd,
});
invalidateCachedGhAuthOnFailure(mergeResult, cwd);
Expand Down
6 changes: 4 additions & 2 deletions src/components/layout/TopBarOpenPR.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1604,9 +1604,11 @@ export function TopBarOpenPR(props: { noDragStyle: CSSProperties }) {
}

setStep("action");
// "default" is resolved to a repository-allowed strategy in the host
// service: `gh pr merge` requires an explicit --merge/--rebase/--squash
// flag because it runs without a TTY here.
const result = await mergePr({
method:
createPrMergeMethod === "default" ? undefined : createPrMergeMethod,
method: createPrMergeMethod,
cwd: workspaceCwd,
});
setStep("idle");
Expand Down
4 changes: 2 additions & 2 deletions src/components/layout/TopBarOpenPR.utils.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { isReasonablePullRequestTitle } from "@/lib/source-control-pr";
import type { PrMergeMethod } from "@/lib/pr-status";
import type { ConcretePrMergeMethod, PrMergeMethod } from "@/lib/pr-status";

export type CreatePrDialogStep =
"idle" | "loading" | "ready" | "committing" | "reviewing" | "pushing" | "creating-pr" | "action";

export type CreatePrSubmitAction = "pr";

export type ConcretePrMergeMethod = Exclude<PrMergeMethod, "default">;
export type { ConcretePrMergeMethod };

export interface RepoMergeSettings {
squashMergeAllowed: boolean;
Expand Down
6 changes: 6 additions & 0 deletions src/lib/pr-status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ export type WorkspacePrStatus =
/** Merge strategy used when Create PR queues GitHub auto-merge. */
export type PrMergeMethod = "default" | "merge" | "squash" | "rebase";

/**
* Strategy actually handed to `gh pr merge`. "default" is not usable there:
* without a TTY the CLI requires one of --merge/--rebase/--squash.
*/
export type ConcretePrMergeMethod = Exclude<PrMergeMethod, "default">;

/** Raw payload returned by the `scm:get-pr-status` IPC handler. */
export interface GitHubPrPayload {
number: number;
Expand Down
3 changes: 2 additions & 1 deletion tests/git-graph-tag-args.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,12 @@ describe("buildCreatePullRequestArgs", () => {
});

describe("buildAutoMergePullRequestArgs", () => {
test("lets GitHub select the repository default merge strategy", () => {
test("always passes an explicit strategy because gh needs one without a TTY", () => {
expect(buildAutoMergePullRequestArgs()).toEqual([
"pr",
"merge",
"--auto",
"--squash",
"--delete-branch",
]);
});
Expand Down
40 changes: 39 additions & 1 deletion tests/scm-runtime-pr.test.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,50 @@
import { describe, expect, test } from "bun:test";
import { ensureGhAuth, invalidateGhAuthCache } from "../electron/host-service/gh-auth";
import { buildAutoMergePullRequestArgs, classifyAutoMergeFailure } from "../electron/host-service/scm-runtime";
import {
buildAutoMergePullRequestArgs,
buildMergePullRequestArgs,
classifyAutoMergeFailure,
pickAllowedMergeMethod,
} from "../electron/host-service/scm-runtime";

describe("Create PR SCM runtime", () => {
test("builds a concrete auto-merge command", () => {
expect(buildAutoMergePullRequestArgs("squash")).toEqual(["pr", "merge", "--auto", "--squash", "--delete-branch"]);
});

test("merge command always carries an explicit strategy flag", () => {
// Regression: `gh pr merge --delete-branch` exits with
// "--merge, --rebase, or --squash required when not running interactively",
// so the Merge PR action must never omit the strategy.
expect(buildMergePullRequestArgs("merge")).toEqual(["pr", "merge", "--merge", "--delete-branch"]);
expect(buildMergePullRequestArgs()).toEqual(["pr", "merge", "--squash", "--delete-branch"]);
});

test("resolves the default merge method against repository settings", () => {
expect(pickAllowedMergeMethod({ method: "rebase" })).toBe("rebase");
expect(pickAllowedMergeMethod({ method: "default" })).toBe("squash");
expect(pickAllowedMergeMethod({})).toBe("squash");
expect(
pickAllowedMergeMethod({
method: "default",
settings: { squashMergeAllowed: false, mergeCommitAllowed: true, rebaseMergeAllowed: true },
}),
).toBe("merge");
expect(
pickAllowedMergeMethod({
method: "default",
settings: { squashMergeAllowed: false, mergeCommitAllowed: false, rebaseMergeAllowed: true },
}),
).toBe("rebase");
// An explicit user choice is honored even if the repo reports it as disallowed.
expect(
pickAllowedMergeMethod({
method: "squash",
settings: { squashMergeAllowed: false, mergeCommitAllowed: true, rebaseMergeAllowed: true },
}),
).toBe("squash");
});

test("classifies graceful auto-merge fallback cases", () => {
expect(classifyAutoMergeFailure("GraphQL: Pull request is in clean status")).toBe("clean-status");
expect(classifyAutoMergeFailure("Auto-merge is disabled for this repository")).toBe("unsupported");
Expand Down
Loading