From b70f9e2d5bcd0e8d0138bb2639562469001133b7 Mon Sep 17 00:00:00 2001 From: Nicolas Medda Date: Wed, 22 Apr 2026 09:44:26 +0000 Subject: [PATCH] BBC2-15 add bb pr merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merges an open PR via POST /pullrequests/{id}/merge with the destination branch's default strategy, or an override via --strategy. Supports --message for a custom merge commit message and --delete for server-side + local source branch cleanup. Strategy resolution: - --strategy validates against the destination branch's merge_strategies allow-list (surfaced client-side with a helpful message listing the permitted set) before the POST, rather than taking an opaque 400 from the API. - Without --strategy, use destination.branch.default_merge_strategy (set via the branch's merge settings in the web UI); fall back to the API's own default merge_commit when unset. Both strategy fields are omitted from the default /pullrequests/{id} serialization. Request them additively via fields=+... (see docs/bb-notes under "Default merge strategy"). That's done in getPullRequest globally — the extra fields are also useful to future consumers (e.g. BBC2-44) and `+` additive syntax doesn't drop existing fields. Response handling: - 200 → synchronous success, print PR URL. - 202 → poll task-status with capped exponential backoff (15 attempts, 500ms → 4s), resolve to SUCCESS or FAILED or time out with a retry message. - 409 / 555 → clean retry-friendly messages from the backend layer. --delete local cleanup is best-effort. Guards: only delete when local branch name matches the PR's source; switch to destination first when currently on the source; fall back to a warning (non-fatal) if git refuses (dirty tree, non-FF, etc.) — the server-side merge already landed. Extends GitRunner with the write primitives checkoutExistingBranch and deleteLocalBranch (plus the hasLocalBranch query), and adds GitError whose message carries git's stderr so users see git's own diagnostics. --- src/backend/pullrequests/index.test.ts | 279 +++++++++++++++++++++++++ src/backend/pullrequests/index.ts | 216 ++++++++++++++++++- src/commands/pullrequest/index.ts | 21 ++ src/commands/pullrequest/merge.ts | 222 ++++++++++++++++++++ src/shared/repository/git.ts | 63 ++++++ src/shared/repository/index.ts | 2 +- src/shared/repository/resolve.test.ts | 32 ++- 7 files changed, 824 insertions(+), 11 deletions(-) create mode 100644 src/commands/pullrequest/merge.ts diff --git a/src/backend/pullrequests/index.test.ts b/src/backend/pullrequests/index.test.ts index e30b5f4..c80f122 100644 --- a/src/backend/pullrequests/index.test.ts +++ b/src/backend/pullrequests/index.test.ts @@ -6,10 +6,12 @@ import { createPullRequest, createPullRequestComment, findOpenPullRequestForBranch, + getMergeTaskStatus, getPullRequest, getPullRequestDiff, listEffectiveDefaultReviewers, listPullRequests, + mergePullRequest, type PullRequest, type PullRequestDetail, PullRequestError, @@ -380,6 +382,8 @@ describe("getPullRequest", () => { "A detailed PR description.\n\n- fix thing\n- fix other thing", sourceBranch: "feature/auth", destinationBranch: "main", + defaultMergeStrategy: null, + allowedMergeStrategies: [], reviewers: [ { account: { uuid: "{bob}", displayName: "Bob", nickname: "bob" }, @@ -415,6 +419,72 @@ describe("getPullRequest", () => { expect((err as PullRequestError).status).toBe(404); }); + test("requests merge-strategy fields additively via fields=+...", async () => { + // Bitbucket omits `default_merge_strategy` and `merge_strategies` + // from the default serialization; bb pr merge needs both. Assert the + // `fields` query param is present so we don't silently stop asking + // for them. + let seenFields: string | null = null; + server.use( + http.get(PR_DETAIL_PATH(42), ({ request }) => { + seenFields = new URL(request.url).searchParams.get("fields"); + return HttpResponse.json(makePrDetail()); + }), + ); + + await getPullRequest(creds, ref, 42); + + expect(seenFields!).toBe( + "+destination.branch.default_merge_strategy,+destination.branch.merge_strategies", + ); + }); + + test("maps destination.branch merge-strategy fields", async () => { + server.use( + http.get(PR_DETAIL_PATH(42), () => + HttpResponse.json( + makePrDetail({ + destination: { + branch: { + name: "main", + default_merge_strategy: "squash", + merge_strategies: [ + "merge_commit", + "squash", + "unknown_future_value", + ], + }, + }, + }), + ), + ), + ); + + const result = await getPullRequest(creds, ref, 42); + expect(result.defaultMergeStrategy).toBe("squash"); + // Unknown strategies are dropped from the allowed list — we'd rather + // refuse to forward a bogus value than silently forward it to the + // API and take a 400 later. + expect(result.allowedMergeStrategies).toEqual(["merge_commit", "squash"]); + }); + + test("default merge strategy is null when server sends an unknown or missing value", async () => { + server.use( + http.get(PR_DETAIL_PATH(42), () => + HttpResponse.json( + makePrDetail({ + destination: { + branch: { name: "main", default_merge_strategy: "bogus" }, + }, + }), + ), + ), + ); + + const result = await getPullRequest(creds, ref, 42); + expect(result.defaultMergeStrategy).toBeNull(); + }); + test("handles a PR with no participants", async () => { server.use( http.get(PR_DETAIL_PATH(42), () => @@ -921,6 +991,215 @@ describe("review action endpoints (approve / unapprove / request-changes / unreq }); }); +describe("mergePullRequest", () => { + const MERGE_PATH = (id: number) => + `${BITBUCKET_BASE}/repositories/ws/repo/pullrequests/${id}/merge`; + + test("POSTs the merge_strategy and returns the merged PR on 200", async () => { + let seenBody: Record | null = null; + server.use( + http.post(MERGE_PATH(42), async ({ request }) => { + seenBody = (await request.json()) as Record; + return HttpResponse.json(makePrDetail({ id: 42, state: "MERGED" }), { + status: 200, + }); + }), + ); + + const result = await mergePullRequest(creds, ref, 42, { + mergeStrategy: "squash", + }); + + expect(seenBody!).toEqual({ + type: "pullrequest_merge_parameters", + merge_strategy: "squash", + }); + expect(result.kind).toBe("done"); + if (result.kind === "done") { + expect(result.pr.id).toBe(42); + expect(result.pr.state).toBe("MERGED"); + } + }); + + test("passes message and close_source_branch through when set", async () => { + let seenBody: Record | null = null; + server.use( + http.post(MERGE_PATH(42), async ({ request }) => { + seenBody = (await request.json()) as Record; + return HttpResponse.json(makePrDetail({ id: 42, state: "MERGED" })); + }), + ); + + await mergePullRequest(creds, ref, 42, { + mergeStrategy: "merge_commit", + message: "custom msg", + closeSourceBranch: true, + }); + + expect(seenBody!).toEqual({ + type: "pullrequest_merge_parameters", + merge_strategy: "merge_commit", + message: "custom msg", + close_source_branch: true, + }); + }); + + test("omits optional fields when not provided", async () => { + let seenBody: Record | null = null; + server.use( + http.post(MERGE_PATH(42), async ({ request }) => { + seenBody = (await request.json()) as Record; + return HttpResponse.json(makePrDetail({ id: 42, state: "MERGED" })); + }), + ); + + await mergePullRequest(creds, ref, 42, { mergeStrategy: "squash" }); + + expect(seenBody!).not.toHaveProperty("message"); + expect(seenBody!).not.toHaveProperty("close_source_branch"); + }); + + test("returns {kind:'async', taskUrl} on 202 with Location header", async () => { + const location = `${MERGE_PATH(42)}/task-status/abc123`; + server.use( + http.post( + MERGE_PATH(42), + () => + new HttpResponse(null, { + status: 202, + headers: { Location: location }, + }), + ), + ); + + const result = await mergePullRequest(creds, ref, 42, { + mergeStrategy: "merge_commit", + }); + + expect(result).toEqual({ kind: "async", taskUrl: location }); + }); + + test("throws PullRequestError on 202 with no Location header", async () => { + server.use( + http.post(MERGE_PATH(42), () => new HttpResponse(null, { status: 202 })), + ); + + const err = await mergePullRequest(creds, ref, 42, { + mergeStrategy: "merge_commit", + }).catch((e) => e); + + expect(err).toBeInstanceOf(PullRequestError); + expect((err as PullRequestError).status).toBe(202); + }); + + test("409 surfaces as a PullRequestError with a retry-friendly message", async () => { + server.use( + http.post(MERGE_PATH(42), () => new HttpResponse(null, { status: 409 })), + ); + + const err = await mergePullRequest(creds, ref, 42, { + mergeStrategy: "merge_commit", + }).catch((e) => e); + + expect(err).toBeInstanceOf(PullRequestError); + expect((err as PullRequestError).status).toBe(409); + expect((err as Error).message).toContain("refs changed"); + }); + + test("555 surfaces as a PullRequestError with a timeout message", async () => { + server.use( + http.post(MERGE_PATH(42), () => + HttpResponse.json( + { type: "error", error: { message: "timed out" } }, + { status: 555 }, + ), + ), + ); + + const err = await mergePullRequest(creds, ref, 42, { + mergeStrategy: "merge_commit", + }).catch((e) => e); + + expect(err).toBeInstanceOf(PullRequestError); + expect((err as PullRequestError).status).toBe(555); + expect((err as Error).message).toContain("timed out"); + }); +}); + +describe("getMergeTaskStatus", () => { + const TASK_URL = + "https://api.bitbucket.org/2.0/repositories/ws/repo/pullrequests/42/merge/task-status/abc"; + + test("maps PENDING task status to { status: 'PENDING' }", async () => { + server.use( + http.get(TASK_URL, () => + HttpResponse.json({ + task_status: "PENDING", + links: { self: { href: TASK_URL } }, + }), + ), + ); + + const result = await getMergeTaskStatus(creds, TASK_URL); + expect(result).toEqual({ status: "PENDING" }); + }); + + test("maps SUCCESS task status to { status:'SUCCESS', pr }", async () => { + server.use( + http.get(TASK_URL, () => + HttpResponse.json({ + task_status: "SUCCESS", + links: { self: { href: TASK_URL } }, + merge_result: makePrDetail({ id: 42, state: "MERGED" }), + }), + ), + ); + + const result = await getMergeTaskStatus(creds, TASK_URL); + expect(result.status).toBe("SUCCESS"); + if (result.status === "SUCCESS") { + expect(result.pr.id).toBe(42); + expect(result.pr.state).toBe("MERGED"); + } + }); + + test("maps any other task status to { status:'FAILED', error }", async () => { + server.use( + http.get(TASK_URL, () => + HttpResponse.json({ + task_status: "FAILED", + error: { message: "conflict at src/x.ts" }, + }), + ), + ); + + const result = await getMergeTaskStatus(creds, TASK_URL); + expect(result).toEqual({ + status: "FAILED", + error: "conflict at src/x.ts", + }); + }); + + test("SUCCESS without merge_result throws a PullRequestError", async () => { + server.use( + http.get(TASK_URL, () => HttpResponse.json({ task_status: "SUCCESS" })), + ); + + const err = await getMergeTaskStatus(creds, TASK_URL).catch((e) => e); + expect(err).toBeInstanceOf(PullRequestError); + }); + + test("non-2xx propagates as PullRequestError with status", async () => { + server.use( + http.get(TASK_URL, () => new HttpResponse(null, { status: 500 })), + ); + + const err = await getMergeTaskStatus(creds, TASK_URL).catch((e) => e); + expect(err).toBeInstanceOf(PullRequestError); + expect((err as PullRequestError).status).toBe(500); + }); +}); + describe("getPullRequestDiff", () => { const DIFF_PATH = (id: number) => `${BITBUCKET_BASE}/repositories/ws/repo/pullrequests/${id}/diff`; diff --git a/src/backend/pullrequests/index.ts b/src/backend/pullrequests/index.ts index 0d7236f..0a810ab 100644 --- a/src/backend/pullrequests/index.ts +++ b/src/backend/pullrequests/index.ts @@ -27,6 +27,26 @@ export type PullRequestState = | "DECLINED" | "SUPERSEDED"; +/** + * Merge strategies recognized by Bitbucket on `POST /pullrequests/{id}/merge`. + * Mirrors the `pullrequest_merge_parameters.merge_strategy` enum (generated + * schema). The list *allowed* on any given destination branch is a subset + * exposed as `PullRequestDetail.allowedMergeStrategies`. + */ +export const MERGE_STRATEGIES = [ + "merge_commit", + "squash", + "fast_forward", + "squash_fast_forward", + "rebase_fast_forward", + "rebase_merge", +] as const; +export type MergeStrategy = (typeof MERGE_STRATEGIES)[number]; + +export function isMergeStrategy(value: string): value is MergeStrategy { + return (MERGE_STRATEGIES as readonly string[]).includes(value); +} + export type PullRequestAuthor = { uuid: string; displayName: string; @@ -65,6 +85,20 @@ export type PullRequestDetail = PullRequest & { description: string; sourceBranch: string; destinationBranch: string; + /** + * The destination branch's configured default merge strategy, or null + * when the branch has no default set (Bitbucket's own fallback is + * `"merge_commit"`). Only populated when the fetch explicitly requests + * `fields=+destination.branch.default_merge_strategy`. + */ + defaultMergeStrategy: MergeStrategy | null; + /** + * Strategies the destination branch permits on merge. Empty array when + * the branch has no restrictions configured (in which case the server + * allows all). Only populated when the fetch explicitly requests + * `fields=+destination.branch.merge_strategies`. + */ + allowedMergeStrategies: MergeStrategy[]; reviewers: Reviewer[]; participants: Participant[]; }; @@ -266,9 +300,25 @@ export async function createPullRequest( } /** - * Fetches a single pull request by id. Single typed call — the overlay - * (BBC2-38) gives us typed path params and response shape, so we stay on - * openapi-fetch's client rather than dropping to raw fetch. + * Fields the default serialization omits, requested additively via Bitbucket's + * `fields=+foo,+bar` mechanism. Both live on `destination.branch` and are + * required for `bb pr merge` — the default strategy and the client-side + * strategy validation list. See docs/bb-notes.md → "Default merge strategy". + * + * Adding to the default response (rather than dropping fields) means + * existing consumers of this function are unaffected. + */ +const PR_EXTRA_FIELDS = [ + "+destination.branch.default_merge_strategy", + "+destination.branch.merge_strategies", +].join(","); + +/** + * Fetches a single pull request by id. Augments the default response with + * merge-strategy fields via `fields=+...` — the typed path params stay + * intact; only the query params need a local `as any` because openapi-fetch's + * generated types declare `query?: never` for this endpoint (see + * docs/bb-notes.md → "Gotcha: `fields` is not declared in the OpenAPI spec"). */ export async function getPullRequest( credentials: Credentials, @@ -285,6 +335,10 @@ export async function getPullRequest( repo_slug: ref.slug, pull_request_id: id, }, + // `fields` isn't in the generated types for this endpoint; see + // docs/bb-notes.md → "Gotcha: `fields` is not declared in the + // OpenAPI spec". Cast to loosen the typed contract just here. + query: { fields: PR_EXTRA_FIELDS } as unknown as undefined, }, }, ); @@ -607,6 +661,142 @@ export async function withdrawRequestChanges( ); } +export type MergeInput = { + mergeStrategy: MergeStrategy; + /** Free-form merge commit message. Ignored by `fast_forward`. Max 128 KiB. */ + message?: string; + /** + * Server-side branch cleanup. When true, Bitbucket deletes the source + * branch from the remote after the merge lands. Local-side cleanup is + * the command layer's responsibility. + */ + closeSourceBranch?: boolean; +}; + +/** + * Outcome of the sync merge POST. The server returns: + * 200 → merge complete, body is the updated PR (mapped to `{kind:"done"}`). + * 202 → merge running async (either because it exceeded the sync timeout + * or the caller opted in). The `Location` header carries the + * task-status polling URL (mapped to `{kind:"async"}`). + * + * 4xx/5xx/555 propagate as `PullRequestError` — callers map them to user + * messages (409 "refs changed, retry", 555 "timed out, retry", etc.). + */ +export type MergeResult = + | { kind: "done"; pr: PullRequestDetail } + | { kind: "async"; taskUrl: string }; + +export async function mergePullRequest( + credentials: Credentials, + ref: { workspace: string; slug: string }, + pullRequestId: number, + input: MergeInput, +): Promise { + const client = createBitbucketClient(credentials); + const { data, response } = await client.POST( + "/repositories/{workspace}/{repo_slug}/pullrequests/{pull_request_id}/merge", + { + params: { + path: { + workspace: ref.workspace, + repo_slug: ref.slug, + pull_request_id: pullRequestId, + }, + }, + body: { + type: "pullrequest_merge_parameters", + merge_strategy: input.mergeStrategy, + ...(input.message !== undefined ? { message: input.message } : {}), + ...(input.closeSourceBranch !== undefined + ? { close_source_branch: input.closeSourceBranch } + : {}), + }, + }, + ); + + if (response.status === 202) { + const location = response.headers.get("location"); + if (!location) { + throw new PullRequestError( + `Merge of pull request #${pullRequestId} returned 202 with no Location header — cannot poll.`, + 202, + ); + } + return { kind: "async", taskUrl: location }; + } + + if (!response.ok || !data) { + throw new PullRequestError( + mergeErrorMessage(pullRequestId, response.status), + response.status, + ); + } + + return { kind: "done", pr: toPullRequestDetail(data as RawPullRequest) }; +} + +function mergeErrorMessage(pullRequestId: number, status: number): string { + switch (status) { + case 409: + return `Failed to merge pull request #${pullRequestId}: refs changed mid-merge (HTTP 409). Re-run after fetching the latest state.`; + case 555: + return `Merge of pull request #${pullRequestId} timed out server-side (HTTP 555). Re-run after giving it a moment.`; + default: + return `Failed to merge pull request #${pullRequestId}: HTTP ${status}.`; + } +} + +export type MergeTaskStatus = + | { status: "PENDING" } + | { status: "SUCCESS"; pr: PullRequestDetail } + | { status: "FAILED"; error: string }; + +/** + * Polls a single merge-task-status URL (the one from the 202 Location + * header). Uses raw fetch because the 202 URL is opaque (built by the + * server; openapi-fetch can't route it as a typed endpoint). + */ +export async function getMergeTaskStatus( + credentials: Credentials, + taskUrl: string, +): Promise { + const response = await fetch(taskUrl, { + method: "GET", + headers: { + Authorization: basicAuthHeader(credentials), + Accept: "application/json", + }, + }); + + if (!response.ok) { + throw new PullRequestError( + `Failed to poll merge task: HTTP ${response.status}.`, + response.status, + ); + } + + const raw = (await response.json()) as Record; + const status = String(raw.task_status ?? ""); + if (status === "SUCCESS") { + const mergeResult = raw.merge_result as RawPullRequest | undefined; + if (!mergeResult) { + throw new PullRequestError( + "Merge task reported SUCCESS but returned no merge_result.", + ); + } + return { status: "SUCCESS", pr: toPullRequestDetail(mergeResult) }; + } + if (status === "PENDING") return { status: "PENDING" }; + // Anything else (typically FAILED / ERROR) surfaces the server's error + // message verbatim. Bitbucket's shape: { type: "error", error: { message } }. + const message = + typeof raw.error?.message === "string" + ? raw.error.message + : `Merge task reported '${status}'.`; + return { status: "FAILED", error: message }; +} + function toPullRequestDetail(pr: RawPullRequest): PullRequestDetail { const base = toPullRequest(pr); const raw = pr as Record; @@ -615,11 +805,31 @@ function toPullRequestDetail(pr: RawPullRequest): PullRequestDetail { description: String(raw.summary?.raw ?? raw.description ?? ""), sourceBranch: String(raw.source?.branch?.name ?? ""), destinationBranch: String(raw.destination?.branch?.name ?? ""), + defaultMergeStrategy: toMergeStrategyOrNull( + raw.destination?.branch?.default_merge_strategy, + ), + allowedMergeStrategies: toMergeStrategyList( + raw.destination?.branch?.merge_strategies, + ), reviewers: toReviewers(raw.participants), participants: toParticipants(raw.participants), }; } +function toMergeStrategyOrNull(raw: unknown): MergeStrategy | null { + if (typeof raw !== "string" || !raw) return null; + return isMergeStrategy(raw) ? raw : null; +} + +function toMergeStrategyList(raw: unknown): MergeStrategy[] { + if (!Array.isArray(raw)) return []; + const out: MergeStrategy[] = []; + for (const entry of raw) { + if (typeof entry === "string" && isMergeStrategy(entry)) out.push(entry); + } + return out; +} + function toReviewers(raw: unknown): Reviewer[] { if (!Array.isArray(raw)) return []; const out: Reviewer[] = []; diff --git a/src/commands/pullrequest/index.ts b/src/commands/pullrequest/index.ts index eb4197f..a7605d9 100644 --- a/src/commands/pullrequest/index.ts +++ b/src/commands/pullrequest/index.ts @@ -4,6 +4,7 @@ import { runPullRequestComment } from "./comment.ts"; import { runPullRequestCreate } from "./create.ts"; import { runPullRequestDiff } from "./diff.ts"; import { runPullRequestList } from "./list.ts"; +import { runPullRequestMerge } from "./merge.ts"; import { runPullRequestReview } from "./review.ts"; import { runPullRequestView } from "./view.ts"; @@ -86,6 +87,26 @@ export function registerPullRequestCommands(program: Command): void { ) .action(withRenderer(runPullRequestDiff)); + pr.command("merge") + .description( + "Merge an open pull request (defaults to the PR for the current branch)", + ) + .argument("[id]", "Pull request number") + .option( + "-R, --repository ", + "Override repository detection", + ) + .option( + "-s, --strategy ", + "Merge strategy: merge_commit, squash, fast_forward, squash_fast_forward, rebase_fast_forward, rebase_merge (default: branch's configured strategy)", + ) + .option("-m, --message ", "Override the merge commit message") + .option( + "--delete", + "Delete the source branch remotely and locally after merging", + ) + .action(withRenderer(runPullRequestMerge)); + pr.command("review") .description( "Submit a review on a pull request (defaults to the PR for the current branch)", diff --git a/src/commands/pullrequest/merge.ts b/src/commands/pullrequest/merge.ts new file mode 100644 index 0000000..bc97345 --- /dev/null +++ b/src/commands/pullrequest/merge.ts @@ -0,0 +1,222 @@ +import { + getMergeTaskStatus, + getPullRequest, + isMergeStrategy, + MERGE_STRATEGIES, + type MergeStrategy, + mergePullRequest, + type PullRequestDetail, + PullRequestError, + type PullRequestState, +} from "../../backend/pullrequests/index.ts"; +import { loadConfigOrExit } from "../../shared/config/index.ts"; +import type { Renderer } from "../../shared/renderer/index.ts"; +import { + defaultGitRunner, + GitError, + type GitRunner, + RepositoryResolutionError, + resolveRepository, +} from "../../shared/repository/index.ts"; +import { resolveCurrentPullRequestId } from "./current.ts"; + +export type PullRequestMergeOptions = { + repository?: string; + strategy?: string; + message?: string; + delete?: boolean; +}; + +const TERMINAL_STATES: ReadonlySet = new Set([ + "MERGED", + "DECLINED", + "SUPERSEDED", +]); + +/** Bitbucket's own fallback when the destination branch has no configured default. */ +const API_DEFAULT_STRATEGY: MergeStrategy = "merge_commit"; + +/** + * Budget for polling an async (202) merge. 15 tries * ~2s backoff ≈ 30s + * before giving up — plenty for ordinary merges, short enough to not feel + * stuck. Callers can always re-run; the task is server-side and persists. + */ +const POLL_MAX_ATTEMPTS = 15; +const POLL_INITIAL_MS = 500; +const POLL_MAX_MS = 4000; + +export async function runPullRequestMerge( + renderer: Renderer, + idArg: string | undefined, + options: PullRequestMergeOptions, + git: GitRunner = defaultGitRunner, +): Promise { + const config = await loadConfigOrExit(renderer); + + try { + const ref = await resolveRepository({ override: options.repository }); + const id = await resolveCurrentPullRequestId(idArg, { + renderer, + config, + ref, + commandName: "merge", + }); + + const pr = await getPullRequest(config, ref, id); + if (TERMINAL_STATES.has(pr.state)) { + renderer.error( + `Pull request #${id} is ${pr.state.toLowerCase()}; cannot merge.`, + ); + process.exit(1); + } + + const strategy = resolveStrategy(renderer, pr, options.strategy); + + const result = await mergePullRequest(config, ref, id, { + mergeStrategy: strategy, + ...(options.message !== undefined ? { message: options.message } : {}), + // Only include close_source_branch when --delete is set. Omitting + // it lets the PR's created-with value apply; passing `false` + // would override it for this merge and could surprise users who + // created the PR with "delete branch on merge" checked. + ...(options.delete ? { closeSourceBranch: true } : {}), + }); + + const merged = + result.kind === "done" + ? result.pr + : await pollUntilDone(config, result.taskUrl, renderer); + + if (renderer.json) { + renderer.detail(merged, []); + } else { + renderer.message(`Merged pull request #${id}: ${merged.url}`); + } + + if (options.delete) { + await cleanupLocalSourceBranch(renderer, git, pr); + } + } catch (err) { + if ( + err instanceof RepositoryResolutionError || + err instanceof PullRequestError + ) { + renderer.error(err.message); + process.exit(1); + } + throw err; + } +} + +function resolveStrategy( + renderer: Renderer, + pr: PullRequestDetail, + raw: string | undefined, +): MergeStrategy { + if (raw !== undefined) { + if (!isMergeStrategy(raw)) { + renderer.error( + `Unknown merge strategy '${raw}'. Allowed: ${MERGE_STRATEGIES.join(", ")}.`, + ); + process.exit(1); + } + // Validate against the branch's allowed list when Bitbucket populated + // it. Empty list means "no restriction" (or the server didn't report + // the field) — fall through and let the API have the final word. + if ( + pr.allowedMergeStrategies.length > 0 && + !pr.allowedMergeStrategies.includes(raw) + ) { + renderer.error( + `Merge strategy '${raw}' is not allowed on branch '${pr.destinationBranch}'. Allowed: ${pr.allowedMergeStrategies.join(", ")}.`, + ); + process.exit(1); + } + return raw; + } + + // No --strategy: use the branch's default; fall back to Bitbucket's own + // default if none configured. + return pr.defaultMergeStrategy ?? API_DEFAULT_STRATEGY; +} + +/** + * Polls a 202 task-status URL with capped exponential backoff until the + * merge reaches a terminal state or we exhaust the attempt budget. Returns + * the merged PR on success; errors surface as PullRequestError from the + * backend and bubble up to runPullRequestMerge. + */ +async function pollUntilDone( + credentials: Parameters[0], + taskUrl: string, + renderer: Renderer, +): Promise { + renderer.message("Merge accepted — polling task status…"); + let delay = POLL_INITIAL_MS; + for (let attempt = 0; attempt < POLL_MAX_ATTEMPTS; attempt++) { + const status = await getMergeTaskStatus(credentials, taskUrl); + if (status.status === "SUCCESS") return status.pr; + if (status.status === "FAILED") { + throw new PullRequestError(`Merge task failed: ${status.error}`); + } + await sleep(delay); + delay = Math.min(delay * 2, POLL_MAX_MS); + } + throw new PullRequestError( + `Merge task did not finish within ${POLL_MAX_ATTEMPTS} polls. Re-run the merge to check its current state.`, + ); +} + +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +/** + * Best-effort local cleanup after `--delete`. Guards against the usual + * footguns: only deletes the local branch if its name matches the PR's + * source (so we never wipe a branch with the same name from a different + * origin); switches off it first when it's currently checked out. + * + * Errors here are non-fatal — the merge already landed. Surface them as + * warnings so the user knows the local branch wasn't cleaned up but the + * command's exit status still reflects the successful merge. + */ +async function cleanupLocalSourceBranch( + renderer: Renderer, + git: GitRunner, + pr: PullRequestDetail, +): Promise { + const cwd = process.cwd(); + const source = pr.sourceBranch; + const destination = pr.destinationBranch; + + if (!source) return; + try { + if (!(await git.hasLocalBranch(cwd, source))) return; + + const current = await git.getCurrentBranch(cwd); + if (current === source) { + // Can't delete the branch we're on — try switching to the + // destination first. If that branch doesn't exist locally or the + // checkout errors (dirty tree), bail out with a warning. + if (!destination || !(await git.hasLocalBranch(cwd, destination))) { + renderer.message( + `Warning: did not delete local branch '${source}' — no local '${destination}' to switch to first. Run 'git checkout && git branch -d ${source}' manually.`, + ); + return; + } + await git.checkoutExistingBranch(cwd, destination); + } + + await git.deleteLocalBranch(cwd, source); + renderer.message(`Deleted local branch '${source}'.`); + } catch (err) { + if (err instanceof GitError) { + renderer.message( + `Warning: could not clean up local branch '${source}': ${err.message}`, + ); + return; + } + throw err; + } +} diff --git a/src/shared/repository/git.ts b/src/shared/repository/git.ts index 628d10d..33aafa5 100644 --- a/src/shared/repository/git.ts +++ b/src/shared/repository/git.ts @@ -1,5 +1,22 @@ import { $ } from "bun"; +/** + * Error from a git command that was expected to succeed. Carries git's stderr + * so callers can surface the underlying message (which is usually more + * actionable than anything bbcli could synthesize) to the user. + */ +export class GitError extends Error { + override name = "GitError"; + readonly stderr: string; + readonly exitCode: number; + + constructor(message: string, exitCode: number, stderr: string) { + super(message); + this.exitCode = exitCode; + this.stderr = stderr; + } +} + /** * Thin wrapper around the git CLI. Injected into resolveRepository so tests * can simulate every failure mode without shelling out. Production code uses @@ -42,6 +59,19 @@ export interface GitRunner { cwd: string, remote: string, ): Promise; + /** Returns true iff a local branch with the given name exists. */ + hasLocalBranch(cwd: string, branch: string): Promise; + /** + * Switches to an existing local branch. Throws GitError on non-zero + * exit (most commonly: a dirty working tree that would be clobbered). + */ + checkoutExistingBranch(cwd: string, branch: string): Promise; + /** + * Deletes a local branch safely (`git branch -d`). Throws GitError on + * non-zero exit, which git returns when the branch hasn't been merged + * anywhere git can see — we never force with `-D` from this wrapper. + */ + deleteLocalBranch(cwd: string, branch: string): Promise; } export const defaultGitRunner: GitRunner = { @@ -120,4 +150,37 @@ export const defaultGitRunner: GitRunner = { const prefix = `${remote}/`; return full.startsWith(prefix) ? full.slice(prefix.length) : full; }, + + async hasLocalBranch(cwd, branch) { + // `show-ref --verify --quiet` exits 0 iff the ref exists; no output. + const result = + await $`git -C ${cwd} show-ref --verify --quiet ${`refs/heads/${branch}`}` + .nothrow() + .quiet(); + return result.exitCode === 0; + }, + + async checkoutExistingBranch(cwd, branch) { + const result = await $`git -C ${cwd} checkout ${branch}`.nothrow().quiet(); + throwIfFailed(result, `git checkout ${branch}`); + }, + + async deleteLocalBranch(cwd, branch) { + const result = await $`git -C ${cwd} branch -d ${branch}`.nothrow().quiet(); + throwIfFailed(result, `git branch -d ${branch}`); + }, }; + +function throwIfFailed( + result: { exitCode: number; stderr: Buffer; stdout: Buffer }, + command: string, +): void { + if (result.exitCode === 0) return; + const stderr = result.stderr.toString().trim(); + const stdout = result.stdout.toString().trim(); + // Git writes most diagnostics to stderr, but a few to stdout; prefer + // stderr, fall back to stdout so we never lose the actual reason. + const message = + stderr || stdout || `${command} exited with code ${result.exitCode}`; + throw new GitError(message, result.exitCode, stderr); +} diff --git a/src/shared/repository/index.ts b/src/shared/repository/index.ts index f6bb27a..4f18d37 100644 --- a/src/shared/repository/index.ts +++ b/src/shared/repository/index.ts @@ -1,5 +1,5 @@ export type { GitRunner } from "./git.ts"; -export { defaultGitRunner } from "./git.ts"; +export { defaultGitRunner, GitError } from "./git.ts"; export type { RepositoryRef } from "./parse-url.ts"; export { parseBitbucketRemoteUrl } from "./parse-url.ts"; export { diff --git a/src/shared/repository/resolve.test.ts b/src/shared/repository/resolve.test.ts index dd78455..f3ca5ed 100644 --- a/src/shared/repository/resolve.test.ts +++ b/src/shared/repository/resolve.test.ts @@ -32,6 +32,11 @@ function fakeGit(s: Setup = {}): GitRunner { async getDefaultBranchFromRemote() { return undefined; }, + async hasLocalBranch() { + return false; + }, + async checkoutExistingBranch() {}, + async deleteLocalBranch() {}, }; } @@ -57,35 +62,48 @@ describe("resolveRepository — override", () => { test("does not consult git when override is given", async () => { let called = false; + const markCalled = () => { + called = true; + }; const git: GitRunner = { async isInsideWorkTree() { - called = true; + markCalled(); return true; }, async listRemotes() { - called = true; + markCalled(); return []; }, async getRemoteUrl() { - called = true; + markCalled(); return undefined; }, async getCurrentBranch() { - called = true; + markCalled(); return undefined; }, async getSha() { - called = true; + markCalled(); return undefined; }, async getRemoteBranchSha() { - called = true; + markCalled(); return undefined; }, async getDefaultBranchFromRemote() { - called = true; + markCalled(); return undefined; }, + async hasLocalBranch() { + markCalled(); + return false; + }, + async checkoutExistingBranch() { + markCalled(); + }, + async deleteLocalBranch() { + markCalled(); + }, }; await resolveRepository({ override: "ws/repo" }, git); expect(called).toBe(false);