diff --git a/electron/host-service/scm-runtime.ts b/electron/host-service/scm-runtime.ts index a09c5c83..7411f3d0 100644 --- a/electron/host-service/scm-runtime.ts +++ b/electron/host-service/scm-runtime.ts @@ -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, @@ -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); @@ -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) { @@ -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 = { + 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 { + 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; @@ -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); @@ -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); diff --git a/src/components/layout/TopBarOpenPR.tsx b/src/components/layout/TopBarOpenPR.tsx index 670a943f..ba99f212 100644 --- a/src/components/layout/TopBarOpenPR.tsx +++ b/src/components/layout/TopBarOpenPR.tsx @@ -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"); diff --git a/src/components/layout/TopBarOpenPR.utils.ts b/src/components/layout/TopBarOpenPR.utils.ts index de859045..71e098cc 100644 --- a/src/components/layout/TopBarOpenPR.utils.ts +++ b/src/components/layout/TopBarOpenPR.utils.ts @@ -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; +export type { ConcretePrMergeMethod }; export interface RepoMergeSettings { squashMergeAllowed: boolean; diff --git a/src/lib/pr-status.ts b/src/lib/pr-status.ts index b379e07b..48696798 100644 --- a/src/lib/pr-status.ts +++ b/src/lib/pr-status.ts @@ -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; + /** Raw payload returned by the `scm:get-pr-status` IPC handler. */ export interface GitHubPrPayload { number: number; diff --git a/tests/git-graph-tag-args.test.ts b/tests/git-graph-tag-args.test.ts index 7955a50c..ff2268a3 100644 --- a/tests/git-graph-tag-args.test.ts +++ b/tests/git-graph-tag-args.test.ts @@ -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", ]); }); diff --git a/tests/scm-runtime-pr.test.ts b/tests/scm-runtime-pr.test.ts index 0a22b0fe..eafede01 100644 --- a/tests/scm-runtime-pr.test.ts +++ b/tests/scm-runtime-pr.test.ts @@ -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");