From 339179f50a84dc599cce373dbb48f172f182f44e Mon Sep 17 00:00:00 2001 From: Bilal Bakr <62337003+Bil0000@users.noreply.github.com> Date: Thu, 15 Oct 2026 07:52:00 +0300 Subject: [PATCH] feat(pr): delete branches after merge or close (#19257) --- .../src/components/AndroidScreenHeader.tsx | 12 +- .../src/components/headerMenu.android.ts | 13 + apps/mobile/src/components/headerMenu.test.ts | 38 ++- .../pull-requests/PullRequestRouteScreen.tsx | 48 +++- .../pull-requests/pullRequestActions.test.ts | 13 + .../pull-requests/pullRequestActions.ts | 7 +- .../pull-requests/usePullRequestAction.ts | 6 +- .../pullRequest/PullRequestService.test.ts | 198 ++++++++++++++ .../src/pullRequest/PullRequestService.ts | 22 +- .../pullRequest/PullRequestDetailPanel.tsx | 48 ++++ .../pullRequestDetail.logic.test.ts | 30 +++ .../pullRequest/pullRequestDetail.logic.ts | 15 ++ .../pullRequest/usePullRequestActions.ts | 4 + packages/contracts/src/pullRequest.test.ts | 15 ++ packages/contracts/src/pullRequest.ts | 2 + .../src/server/AzureDevOpsPullRequestCli.ts | 1 + .../src/client/definition.ts | 1 + .../src/server/GitHubPullRequestApi.test.ts | 246 ++++++++++++++++++ .../src/server/GitHubPullRequestApi.ts | 178 +++++++++++++ .../server/GitHubPullRequestProvider.test.ts | 44 ++++ .../src/server/GitHubPullRequestProvider.ts | 2 + .../src/server/gitHubPullRequestJson.test.ts | 3 + .../src/server/gitHubPullRequestJson.ts | 29 ++- .../src/server/GitLabPullRequestCli.ts | 1 + 24 files changed, 963 insertions(+), 13 deletions(-) diff --git a/apps/mobile/src/components/AndroidScreenHeader.tsx b/apps/mobile/src/components/AndroidScreenHeader.tsx index 624c6d6d939fd5c3552576532edb3e7b6ffbfdbd..1ae2464e0a7713c9a14c6f94b08eaabae1ce2edf 100644 --- a/apps/mobile/src/components/AndroidScreenHeader.tsx +++ b/apps/mobile/src/components/AndroidScreenHeader.tsx @@ -6,6 +6,7 @@ import { AppText as Text } from "./AppText"; import { cn } from "../lib/cn"; import { MaterialIconButton } from "./MaterialIconButton"; import { AndroidAnchoredMenu } from "./AndroidAnchoredMenu"; +import { splitAndroidHeaderActions } from "./headerMenu.android"; import { useScaledTextRole } from "../features/settings/appearance/useScaledTextRole"; import { useMaterialToolbarLayout } from "./useMaterialToolbarLayout"; @@ -16,6 +17,8 @@ export interface AndroidHeaderAction { readonly disabled?: boolean; readonly loading?: boolean; readonly selected?: boolean; + readonly destructive?: boolean; + readonly overflowOnly?: boolean; } export function AndroidHeaderIconButton(props: { @@ -45,10 +48,10 @@ export function AndroidScreenHeader(props: { props.embedded, ); const [headerWidth, setHeaderWidth] = useState(0); - const actions = props.actions ?? []; - const directCount = actions.length > 2 ? (headerWidth >= 600 ? 3 : 1) : actions.length; - const visibleActions = actions.slice(0, directCount); - const overflowActions = actions.slice(directCount); + const { visibleActions, overflowActions } = splitAndroidHeaderActions( + props.actions ?? [], + headerWidth >= 600, + ); return ( ( + actions: ReadonlyArray, + wide: boolean, +) { + const inline = actions.filter((action) => !action.overflowOnly); + const directCount = actions.length > 2 ? (wide ? 3 : 1) : inline.length; + const visibleActions = inline.slice(0, directCount); + return { + visibleActions, + overflowActions: actions.filter((action) => !visibleActions.includes(action)), + }; +} diff --git a/apps/mobile/src/components/headerMenu.test.ts b/apps/mobile/src/components/headerMenu.test.ts index bbc0b1d1ee4daf9a251556b1fd92ae1580a44157..c63f0165019a445dfc7b6f972a881b6a89e25ffa 100644 --- a/apps/mobile/src/components/headerMenu.test.ts +++ b/apps/mobile/src/components/headerMenu.test.ts @@ -1,5 +1,9 @@ import { describe, expect, it } from "vite-plus/test"; -import { androidHeaderMenuActions, findHeaderMenuAction } from "./headerMenu.android"; +import { + androidHeaderMenuActions, + findHeaderMenuAction, + splitAndroidHeaderActions, +} from "./headerMenu.android"; import type { ScreenHeaderMenuItem } from "./ScreenHeader.types"; const items: ReadonlyArray = [ @@ -63,4 +67,36 @@ describe("header menus", () => { expect(findHeaderMenuAction(items, "sessions")).toBeUndefined(); expect(findHeaderMenuAction(items, "removed-session")).toBeUndefined(); }); + + it("keeps overflow-only Android header actions in the overflow menu", () => { + const action = (accessibilityLabel: string, overflowOnly?: boolean) => ({ + accessibilityLabel, + icon: "trash" as const, + onPress: () => {}, + overflowOnly, + }); + const labels = ( + split: ReturnType>>, + ) => ({ + visible: split.visibleActions.map((item) => item.accessibilityLabel), + overflow: split.overflowActions.map((item) => item.accessibilityLabel), + }); + const open = action("Open"); + const remove = action("Delete", true); + expect(labels(splitAndroidHeaderActions([open, action("Comment")], false))).toEqual({ + visible: ["Open", "Comment"], + overflow: [], + }); + expect(labels(splitAndroidHeaderActions([open, remove], true))).toEqual({ + visible: ["Open"], + overflow: ["Delete"], + }); + expect(labels(splitAndroidHeaderActions([remove], true))).toEqual({ + visible: [], + overflow: ["Delete"], + }); + expect( + labels(splitAndroidHeaderActions([action("Review"), action("Comment"), open, remove], true)), + ).toEqual({ visible: ["Review", "Comment", "Open"], overflow: ["Delete"] }); + }); }); diff --git a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx index 37fa732930a03f530124b4d2f478d0b03420920e..f68a19bbf28e752f94939187238680c1df27c541 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx @@ -17,7 +17,12 @@ import { PR_ROW_EXIT, PR_POSITION_LAYOUT, } from "./pullRequestAnimations"; -import { openOnProviderLabel } from "./pullRequestActions"; +import { + canPerformPullRequestAction, + openOnProviderLabel, + PR_ACTION_ICONS, + PR_ACTION_LABELS, +} from "./pullRequestActions"; import * as Clipboard from "expo-clipboard"; import { ControlPillMenu } from "../../components/ControlPillMenu"; import { MaterialIconButton } from "../../components/MaterialIconButton"; @@ -25,6 +30,7 @@ import { MaterialButton } from "../../components/MaterialButton"; import { usePullRequestReviewDraft } from "./usePullRequestReviewDraft"; import { usePullRequestFilesViewed } from "./usePullRequestFilesViewed"; import { usePullRequestPermissions } from "./usePullRequestPermissions"; +import { usePullRequestAction } from "./usePullRequestAction"; import { annotatePullRequestDiff, reviewPosition } from "./pullRequestDiffComments"; import { usePullRequestFileFilter } from "./usePullRequestFileFilter"; import { isPullRequestTestFile } from "./pullRequestFileFilters"; @@ -341,6 +347,15 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps + pr && + action.confirm( + "delete-branch", + PR_ACTION_LABELS["delete-branch"], + `Delete ${pr.headBranch} from ${pr.headRepositoryNameWithOwner ?? pr.repository}? Local branches and worktrees are not changed.`, + ); const stack = usePullRequestStack(params, pr); const refreshActivity = useAtomRefresh(composerPullRequests.activity(requestTarget(params))); const refreshGuide = useAtomRefresh(composerPullRequests.reviewGuide(requestTarget(params))); @@ -417,6 +432,18 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps ) : null} @@ -456,6 +483,25 @@ export function PullRequestRouteScreen({ route }: StaticScreenProps ) : null} + {showsDeleteBranch ? ( + + + {PR_ACTION_LABELS["delete-branch"]} + + + ) : null} { expect(mergeSummary(unknown, true)).toBe("Squash"); }); + it("offers deleting the source branch only once merged or closed on a host that supports it", () => { + const capabilities = { ...detail().capabilities, deleteBranch: true }; + expect(canPerformPullRequestAction(detail({ capabilities }), "delete-branch")).toBe(false); + for (const state of ["merged", "closed"] as const) { + expect(canPerformPullRequestAction(detail({ state, capabilities }), "delete-branch")).toBe( + true, + ); + expect(canPerformPullRequestAction(detail({ state }), "delete-branch")).toBe(false); + } + const refused = detail({ state: "merged", capabilities, viewerPermissions: { actions: [] } }); + expect(canPerformPullRequestAction(refused, "delete-branch")).toBe(false); + }); + it("cannot merge when the repository has disabled every offered strategy", () => { const pr = detail({ mergeCapabilities: { merge: false, squash: false, rebase: false } }); expect(canPerformPullRequestAction(pr, "merge")).toBe(false); diff --git a/apps/mobile/src/features/pull-requests/pullRequestActions.ts b/apps/mobile/src/features/pull-requests/pullRequestActions.ts index 8ce458ae62b5e74a6f0066330a18fdf8cf24d460..359cbf436333695b35f874d84f45aebc98322311 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestActions.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestActions.ts @@ -13,7 +13,7 @@ type ActionDetail = Pick< > & { readonly capabilities: Pick< PullRequestDetail["capabilities"], - "actions" | "mergeMethods" | "updateMethods" + "actions" | "mergeMethods" | "updateMethods" | "deleteBranch" >; readonly viewerPermissions: Pick< PullRequestDetail["viewerPermissions"], @@ -41,6 +41,7 @@ export const PR_ACTION_LABELS = { "enable-auto-merge": "Enable auto-merge", "disable-auto-merge": "Disable auto-merge", revert: "Create revert pull request", + "delete-branch": "Delete branch", "approve-workflows": "Approve workflows", } as const satisfies Record; @@ -54,6 +55,7 @@ export const PR_ACTION_ICONS = { "enable-auto-merge": "clock", "disable-auto-merge": "xmark", revert: "arrow.uturn.backward", + "delete-branch": "trash", "approve-workflows": "checkmark", } as const satisfies Record; @@ -68,6 +70,7 @@ export const ALL_PR_ACTIONS: readonly PullRequestAction[] = [ "close", "reopen", "revert", + "delete-branch", ]; export function allowedMergeMethods(pr: ActionDetail) { @@ -86,6 +89,8 @@ export function canPerformPullRequestAction(pr: ActionDetail, action: PullReques return false; if (action === "reopen") return pr.state === "closed"; if (action === "revert") return pr.state === "merged"; + if (action === "delete-branch") + return pr.state !== "open" && pr.capabilities.deleteBranch === true; if (pr.state !== "open") return false; switch (action) { case "update-branch": diff --git a/apps/mobile/src/features/pull-requests/usePullRequestAction.ts b/apps/mobile/src/features/pull-requests/usePullRequestAction.ts index 870bcbed397654725c3481ec182c2287adf4901f..2b0193ca3081e4fbd92a23d5efab4ef7bdde96f5 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestAction.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestAction.ts @@ -14,7 +14,7 @@ import { pullRequestTarget, type PullRequestParams } from "./usePullRequestDiff" import { usePullRequestPermissions } from "./usePullRequestPermissions"; /** Small row actions stay in place, preserving expanded rows and scroll position. */ -export function usePullRequestAction(params: PullRequestParams, pr: PullRequestDetail) { +export function usePullRequestAction(params: PullRequestParams, pr: PullRequestDetail | null) { const target = pullRequestTarget(params); const permissions = usePullRequestPermissions(params, pr); const run = useAtomCommand(composerPullRequests.runAction, { reportFailure: false }); @@ -23,7 +23,7 @@ export function usePullRequestAction(params: PullRequestParams, pr: PullRequestD const pending = useRef(false); const [busy, setBusy] = useState(false); const available = (action: PullRequestAction) => - !busy && permissions.canWrite && canPerformPullRequestAction(pr, action); + pr !== null && !busy && permissions.canWrite && canPerformPullRequestAction(pr, action); const perform = async (action: PullRequestAction, updateMethod?: PullRequestUpdateMethod) => { if (pending.current || !available(action)) return; pending.current = true; @@ -51,7 +51,7 @@ export function usePullRequestAction(params: PullRequestParams, pr: PullRequestD { text: "Cancel", style: "cancel" }, { text: title, - style: action === "close" ? "destructive" : "default", + style: action === "close" || action === "delete-branch" ? "destructive" : "default", onPress: () => void perform(action), }, ]); diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 370967c0fb29c471545c6b67c51bc8e37d89ef4d..70865973854210438cc8dfaca98f62371c6ef822 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -13,7 +13,9 @@ import * as Exit from "effect/Exit"; import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; import * as Option from "effect/Option"; +import * as Redacted from "effect/Redacted"; import * as Stream from "effect/Stream"; +import { HttpClient, HttpClientResponse } from "effect/http"; import * as TestClock from "effect/testing/TestClock"; import type { OrchestrationProjectShell, @@ -34,6 +36,12 @@ import * as SourceControlProviderRegistry from "../sourceControl/SourceControlPr import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit"; import * as ForgejoCli from "@t3tools/source-control-forgejo/server/ForgejoCli"; import * as ForgejoPullRequestProvider from "@t3tools/source-control-forgejo/server/ForgejoPullRequestProvider"; +import * as GitHubApi from "@t3tools/source-control-github/server/GitHubApi"; +import * as GitHubCredentials from "@t3tools/source-control-github/server/GitHubCredentials"; +import * as GitHubPullRequestApi from "@t3tools/source-control-github/server/GitHubPullRequestApi"; +import * as GitHubPullRequestProvider from "@t3tools/source-control-github/server/GitHubPullRequestProvider"; +import * as GitHubQuota from "@t3tools/source-control-github/server/GitHubQuota"; +import * as TestSourceControlHost from "@t3tools/source-control-testing/TestSourceControlHost"; import { PullRequestProviderError, type ProviderChangeRequest, @@ -1624,6 +1632,196 @@ it.effect("refreshes every reader before a queued merge confirmation finishes", ), ); +it.effect("runs branch deletion only when the host and viewer support it, never for a stack", () => + Effect.gen(function* () { + let actions = 0; + let supported = false; + let allowed = true; + const base = fakeProvider(SourceControlProviderKind.make("github")); + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + { + ...base, + capabilities: { + ...base.capabilities, + actions: ["delete-branch"], + stackActions: true, + get deleteBranch() { + return supported; + }, + }, + getViewerPermissions: () => + Effect.succeed({ + actions: allowed ? ["delete-branch"] : [], + comment: true, + resolve: true, + verdicts: [], + requestReviewers: false, + }), + runAction: (input) => + Effect.sync(() => { + assert.strictEqual(input.action, "delete-branch"); + actions++; + }), + }, + ], + }); + const input = { + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + action: "delete-branch" as const, + }; + assert.strictEqual( + (yield* Effect.flip(service.runAction(input)))._tag, + "PullRequestOperationError", + ); + assert.strictEqual(actions, 0); + supported = true; + assert.strictEqual( + (yield* Effect.flip( + service.runAction({ + ...input, + stackNumber: 2, + expectedStackHeads: [{ number: 1, headSha: "abc123" }], + }), + ))._tag, + "PullRequestOperationError", + ); + assert.strictEqual(actions, 0); + allowed = false; + assert.strictEqual( + (yield* Effect.flip(service.runAction(input)))._tag, + "PullRequestOperationError", + ); + assert.strictEqual(actions, 0); + allowed = true; + yield* service.runAction(input); + assert.strictEqual(actions, 1); + }), +); + +it.effect("keeps branch deletion on the credential that passed source repository permissions", () => + Effect.gen(function* () { + let activeToken = "account-a"; + const requests: Array<{ operation: string; authorization: string | undefined }> = []; + const credentials = Layer.succeed( + GitHubCredentials.GitHubCredentials, + GitHubCredentials.GitHubCredentials.of({ + get: (host) => + Effect.sync(() => ({ + host, + token: Redacted.make(activeToken), + source: "gh" as const, + fingerprint: `${host}:${activeToken}`, + })), + invalidate: () => Effect.void, + }), + ); + const http = Layer.succeed( + HttpClient.HttpClient, + HttpClient.make((request) => + Effect.sync(() => { + const operation = request.url.endsWith("/user") + ? "user" + : request.body._tag === "Uint8Array" + ? new TextDecoder().decode(request.body.body) + : ""; + requests.push({ operation, authorization: request.headers["authorization"] }); + let body; + if (operation === "user") { + body = { id: activeToken === "account-a" ? 101 : 202, login: activeToken }; + } else if (operation.includes("query PullRequestBranchToDelete")) { + body = { + data: { + repository: { + pullRequest: { + state: "MERGED", + headRefName: "feature", + baseRefName: "main", + headRefOid: "abc123", + headRepository: { id: "FORK_REPO" }, + }, + }, + }, + }; + } else if (operation.includes("query PullRequestSourceBranch")) { + body = { + data: { + node: { + defaultBranchRef: { name: "main" }, + ref: { target: { oid: "abc123" } }, + }, + }, + }; + } else if (operation.includes("updateRefs(")) { + body = { data: { updateRefs: {} } }; + } else { + activeToken = "account-b"; + body = { + data: { + repository: { + mergeCommitAllowed: true, + squashMergeAllowed: true, + rebaseMergeAllowed: true, + viewerPermission: "READ", + pullRequest: { + viewerCanUpdate: false, + viewerDidAuthor: false, + headRepository: { viewerPermission: "WRITE" }, + }, + }, + }, + }; + } + return HttpClientResponse.fromWeb(request, new Response(JSON.stringify(body))); + }), + ), + ); + const provider = yield* GitHubPullRequestProvider.make.pipe( + Effect.provide( + GitHubPullRequestApi.layer.pipe( + Layer.provide( + GitHubApi.layer.pipe( + Layer.provide(Layer.mergeAll(credentials, http)), + Layer.provide(GitHubQuota.layer), + Layer.provide(SourceControlRateLimit.layer), + ), + ), + Layer.provide(TestSourceControlHost.layer()), + Layer.provide(NodeServices.layer), + ), + ), + ); + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [provider], + }); + yield* service.runAction({ + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + action: "delete-branch", + }); + assert.strictEqual(activeToken, "account-b"); + assert.strictEqual( + requests.filter(({ operation }) => operation.includes("updateRefs(")).length, + 1, + ); + assert.deepStrictEqual( + requests.map(({ authorization }) => authorization), + [ + "Bearer account-a", + "Bearer account-a", + "Bearer account-a", + "Bearer account-a", + "Bearer account-a", + ], + ); + }), +); + it.effect("refuses an action this viewer may not take, and says what access it takes", () => Effect.gen(function* () { let ran: string | null = null; diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index dd60462d5f8c323201577a6f35ff4cc28a35d4ee..659e0605f66ea1b3e3d98a0b8d048fbac5517cd0 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -356,6 +356,7 @@ const ACTION_ACCESS_REFUSALS: Record = { "disable-auto-merge": "You need write access on this repository to stop it being merged for you once it is ready.", revert: "You need write access on this repository to open a revert pull request.", + "delete-branch": "You need write access on the source repository to delete its branch.", "approve-workflows": "You need write access on this repository to approve workflows from a fork pull request.", }; @@ -2019,6 +2020,14 @@ export const make = Effect.gen(function* () { const runAction = (input: PullRequestActionInput): Effect.Effect => requireProject(input).pipe( Effect.flatMap((project): Effect.Effect => { + if (input.action === "delete-branch" && project.api.capabilities.deleteBranch !== true) { + return Effect.fail( + new PullRequestOperationError({ + operation: "runAction", + detail: "This host cannot delete a change request's source branch.", + }), + ); + } if ( input.stackNumber !== undefined && (project.api.capabilities.stackActions !== true || @@ -2073,7 +2082,7 @@ export const make = Effect.gen(function* () { // What the host can do and what this account may ask of it are two questions, and both // have to say yes. The second is asked last, because it costs a request and the checks // above do not. - return viewerPermissionsOf( + const operation = viewerPermissionsOf( project, input, "runAction", @@ -2158,6 +2167,17 @@ export const make = Effect.gen(function* () { ); }), ); + return input.action === "delete-branch" && project.api.withVerifiedCredential !== undefined + ? project.api + .withVerifiedCredential( + { cwd: project.project.workspaceRoot, host: project.host }, + () => operation.pipe(Effect.result), + ) + .pipe( + Effect.mapError(toPullRequestError("runAction")), + Effect.flatMap(Effect.fromResult), + ) + : operation; }), ); diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 92dd64d298ed395f45abb886c7dc230eb3dd2c7b..e85ac9655139e08024e6970cc1d0a8984847c3f5 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -45,6 +45,7 @@ import { PlayIcon, RotateCcwIcon, SparklesIcon, + Trash2Icon, TriangleAlertIcon, } from "lucide-react"; import { @@ -161,6 +162,7 @@ import { resolveDisplayedPullRequestDetail, resolvePullRequestPrimaryControl, allowsSinglePullRequestMerge, + canDeletePullRequestBranch, resolveBaseFreshness, resolvePullRequestMergeMethod, type PullRequestFinding, @@ -204,6 +206,7 @@ const ACTION_FAILURE_LABELS: Record = { "enable-auto-merge": "Could not turn on auto-merge", "disable-auto-merge": "Could not turn off auto-merge", revert: "Could not open a revert pull request", + "delete-branch": "Could not delete this branch", "approve-workflows": "Could not approve workflows", }; @@ -228,6 +231,8 @@ const ACTION_FAILURE_HINTS: Record = { "The host refused it. Check that you have write access, and that the merge has not already happened.", revert: "The host refused it. Check that you have write access and that this pull request was merged on the host.", + "delete-branch": + "The host refused it. Check that you have write access to the source repository and that the branch has not moved since this page loaded.", "approve-workflows": "The host refused it. Check that you have Actions write access and that these workflow runs are still awaiting approval.", }; @@ -1030,6 +1035,9 @@ export function PullRequestDetailPanel({ if (action === "revert") { toastManager.add({ type: "success", title: "Revert pull request opened" }); } + if (action === "delete-branch") { + toastManager.add({ type: "success", title: "Branch deleted" }); + } // A branch update moves the head commit, which leaves the diff atom pointed at a comparison // that no longer exists — the same staleness the manual refresh button fixes, so it goes // through that path rather than a second one. Every other action here only changes metadata; @@ -1639,6 +1647,22 @@ export function PullRequestDetailPanel({ const statePresentation = detail ? resolvePullRequestState({ state: detail.state, isDraft: detail.isDraft }) : null; + const showsDeleteBranch = detail !== null && canDeletePullRequestBranch(detail); + const environmentConnected = environments.some( + (environment) => + environment.environmentId === environmentId && environment.connection.phase === "connected", + ); + const confirmDeleteBranch = async () => { + if (detail === null) return; + const confirmed = await readLocalApi()?.dialogs.confirm( + [ + `Delete branch ${detail.headBranch}?`, + `This deletes ${detail.headBranch} from ${detail.headRepositoryNameWithOwner ?? detail.repository}. Local branches and worktrees are not changed.`, + ].join("\n"), + { variant: "destructive" }, + ); + if (confirmed) void perform("delete-branch"); + }; const showsApproveWorkflows = workflowApprovalsRequired > 0 && !checksStale && can("approve-workflows"); const checksSummary = checksStale @@ -2424,6 +2448,30 @@ export function PullRequestDetailPanel({ ) : null} + {showsDeleteBranch ? ( + <> + + void confirmDeleteBranch()} + > + + + + {pendingAction === "delete-branch" + ? "Deleting branch..." + : "Delete branch"} + + + {environmentConnected + ? detail.headBranch + : "Reconnect to delete this branch"} + + + + + ) : null} diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index f3054b6e9734b9729c196fa7983cc27fbd03522f..afc78b0c6e226ab7a6854f5f5a36b21f4cab06f6 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -10,6 +10,7 @@ import { type PullRequestDetailView, type PullRequestRef, type PullRequestReviewThread, + type PullRequestState, type RepositoryIdentity, type ThreadPullRequestLink, } from "@t3tools/contracts"; @@ -28,6 +29,7 @@ import { buildExplainPullRequestHandoff, buildPullRequestReferenceContext, buildFixFindingHandoff, + canDeletePullRequestBranch, buildFixFindingsHandoff, groupPullRequestTimelineConversations, handoffPrompt, @@ -1654,12 +1656,40 @@ describe("which actions need the host read again after they run", () => { "disable-auto-merge", "merge", "revert", + "delete-branch", ] as const) { expect(pullRequestActionNeedsHostRefresh(action)).toBe(false); } }); }); +describe("when the source branch can be deleted", () => { + const branchDetail = ( + state: PullRequestState, + overrides: { deleteBranch?: boolean; allowed?: boolean } = {}, + ) => ({ + state, + capabilities: { + deleteBranch: overrides.deleteBranch ?? true, + actions: ["delete-branch" as const], + }, + viewerPermissions: { + actions: overrides.allowed === false ? [] : (["delete-branch"] as const), + }, + }); + + it("offers it only after the pull request is merged or closed", () => { + expect(canDeletePullRequestBranch(branchDetail("merged"))).toBe(true); + expect(canDeletePullRequestBranch(branchDetail("closed"))).toBe(true); + expect(canDeletePullRequestBranch(branchDetail("open"))).toBe(false); + }); + + it("needs both the host capability and the viewer's permission", () => { + expect(canDeletePullRequestBranch(branchDetail("merged", { deleteBranch: false }))).toBe(false); + expect(canDeletePullRequestBranch(branchDetail("merged", { allowed: false }))).toBe(false); + }); +}); + describe("the compact row's single action slot", () => { const check = (status: PullRequestCheck["status"]): PullRequestCheck => ({ name: "ci", diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index 8e209b66d534c29f7961e1d6dec5d2c375e7f550..b20b809d16aca463289ad7a50723a786866a7b50 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -1339,6 +1339,7 @@ const ACTION_NEEDS_HOST_REFRESH: Record = { "enable-auto-merge": false, "disable-auto-merge": false, revert: false, + "delete-branch": false, "approve-workflows": true, }; @@ -1346,6 +1347,20 @@ export function pullRequestActionNeedsHostRefresh(action: PullRequestAction): bo return ACTION_NEEDS_HOST_REFRESH[action]; } +export function canDeletePullRequestBranch( + detail: Pick & { + readonly capabilities: Pick; + readonly viewerPermissions: Pick; + }, +): boolean { + return ( + detail.state !== "open" && + detail.capabilities.deleteBranch === true && + detail.capabilities.actions.includes("delete-branch") && + detail.viewerPermissions.actions.includes("delete-branch") + ); +} + type SnapshotStorage = Pick; export function resolvePullRequestReferenceHost( diff --git a/apps/web/src/components/pullRequest/usePullRequestActions.ts b/apps/web/src/components/pullRequest/usePullRequestActions.ts index 3ca753df2b82a3b2c41f37ddc2911a62dfcfe30f..a54b3efc43a65018c3dbfd379774c80ca8485b2e 100644 --- a/apps/web/src/components/pullRequest/usePullRequestActions.ts +++ b/apps/web/src/components/pullRequest/usePullRequestActions.ts @@ -87,6 +87,7 @@ const ACTION_SUCCESS_LABELS: Record = { "enable-auto-merge": "Auto-merge enabled", "disable-auto-merge": "Auto-merge disabled", revert: "Revert pull request opened", + "delete-branch": "Branch deleted", "approve-workflows": "Workflows approved", }; @@ -101,6 +102,7 @@ const ACTION_FAILURE_LABELS: Record = { "enable-auto-merge": "Could not enable auto-merge", "disable-auto-merge": "Could not disable auto-merge", revert: "Could not open a revert pull request", + "delete-branch": "Could not delete this branch", "approve-workflows": "Could not approve workflows", }; @@ -120,6 +122,8 @@ const ACTION_FAILURE_HINTS: Record = { "disable-auto-merge": "The host refused it. Check that you have write access to this repository.", revert: "The host refused it. Check that you have write access and that this pull request was merged on the host.", + "delete-branch": + "The host refused it. Check that you have write access to the source repository and that the branch has not moved since this page loaded.", "approve-workflows": "The host refused it. Check that you have Actions write access and that these workflow runs are still awaiting approval.", }; diff --git a/packages/contracts/src/pullRequest.test.ts b/packages/contracts/src/pullRequest.test.ts index 2360d232895d4cab3f7f8e76af73efca80b350f4..c83a7fb92822bacc56a3bcf93d5dd470d35d3d9b 100644 --- a/packages/contracts/src/pullRequest.test.ts +++ b/packages/contracts/src/pullRequest.test.ts @@ -220,6 +220,16 @@ describe("leaving a merge for the host to make once it is ready", () => { }); describe("reverting a merged pull request", () => { + it("carries a standalone branch deletion action", () => { + expect( + decodeAction({ + projectId: "project-1", + repository: "acme/web", + number: 7, + action: "delete-branch", + }).action, + ).toBe("delete-branch"); + }); it("carries the revert action without merge options", () => { const action = decodeAction({ projectId: "project-1", @@ -261,6 +271,11 @@ describe("PullRequestCapabilities", () => { it("decodes a server that says nothing about reactions as a server with none", () => { expect(decodeCapabilities(base).reactions).toBeUndefined(); }); + + it("only offers branch deletion when the server advertises it", () => { + expect(decodeCapabilities(base).deleteBranch).toBeUndefined(); + expect(decodeCapabilities({ ...base, deleteBranch: true }).deleteBranch).toBe(true); + }); }); describe("naming the reader as the author to narrow by", () => { diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 2ff64a56e6e23dc19864e9a4c82add23433cddcc..57f775f297476d7952ff58ad5616c1f80ad4dbbe 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -83,6 +83,7 @@ export const PullRequestAction = Schema.Literals([ "draft", "close", "reopen", + "delete-branch", /** Bring the base branch's commits into this one, which is what unblocks a stale branch. */ "update-branch", /** @@ -418,6 +419,7 @@ export const PullRequestCapabilities = Schema.Struct({ actions: Schema.Array(PullRequestAction), /** Merge strategies the provider itself offers, before repository settings narrow them. */ mergeMethods: Schema.Array(PullRequestMergeMethod), + deleteBranch: Schema.optional(Schema.Boolean), /** * How this host can bring a stale branch up to date. Absent where it cannot at all, which is * every host that has not said otherwise — so a provider that says nothing offers nothing. diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts index b7d719725fe3f15cdc71c9f31a2cb7cd71b82ffb..c2fbbfbe80733d45c324004c094d443dc50d129b 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.ts @@ -325,6 +325,7 @@ function actionArgs( // Never reached: this host does not declare the action, so the service refuses it first. case "revert": case "approve-workflows": + case "delete-branch": throw new Error(`Azure DevOps pull request action ${action} is unsupported`); } } diff --git a/packages/source-control-github/src/client/definition.ts b/packages/source-control-github/src/client/definition.ts index 9d20128f8df0a8278afda8c9953e05c97fc5af88..efae9eaa572b061fe3f514cfb3fc738f347546e2 100644 --- a/packages/source-control-github/src/client/definition.ts +++ b/packages/source-control-github/src/client/definition.ts @@ -72,6 +72,7 @@ export const definition = defineSourceControlClient({ "draft", "close", "reopen", + "delete-branch", "update-branch", "enable-auto-merge", "disable-auto-merge", diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index a705cdb2edce98edf84ffceca275a9ae635b1115..0881add16e80967eee8b9b3b36bc2d1f511ca833 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -18,6 +18,7 @@ import * as GitHubQuota from "./GitHubQuota.ts"; import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit"; import * as TestSourceControlHost from "@t3tools/source-control-testing/TestSourceControlHost"; import * as GitHubPullRequestApi from "./GitHubPullRequestApi.ts"; +import * as GitHubPullRequestProvider from "./GitHubPullRequestProvider.ts"; import { KnownWorkflowRuns } from "./gitHubConditionalChecks.ts"; const encodeJson = Schema.encodeSync(Schema.fromJsonString(Schema.Unknown)); @@ -2226,6 +2227,249 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); + it.effect.each([ + { + description: "base-only write", + basePermission: "WRITE", + sourcePermission: "READ", + headRef: { id: "REF" }, + expected: false, + actionAllowed: false, + }, + { + description: "source-only write", + basePermission: "READ", + sourcePermission: "WRITE", + headRef: { id: "REF" }, + expected: true, + actionAllowed: true, + }, + { + description: "a deleted branch", + basePermission: "READ", + sourcePermission: "WRITE", + headRef: null, + expected: false, + actionAllowed: true, + }, + { + description: "an unavailable source repository", + basePermission: "WRITE", + sourcePermission: null, + headRef: { id: "REF" }, + expected: false, + actionAllowed: false, + }, + ])( + "uses source branch access in both permission reads for $description", + ({ basePermission, sourcePermission, headRef, expected, actionAllowed }) => + Effect.gen(function* () { + const answer = coreResponse({ + headRepository: sourcePermission === null ? null : { viewerPermission: sourcePermission }, + headRef, + }); + answer.data.repository.viewerPermission = basePermission; + mockedExecute.mockReturnValue(Effect.succeed(output(encodeJson(answer)))); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const input = { cwd: "/w", repository: "acme/web", host: "github.com", number: 7 }; + expect((yield* cli.getViewerAccess(input)).canDeleteBranch).toBe(actionAllowed); + expect((yield* cli.getPullRequestDetail(input)).viewerAccess.canDeleteBranch).toBe( + expected, + ); + for (const [call] of mockedExecute.mock.calls) { + if ( + call.kind === "graphql" && + !call.query.includes("query PullRequestStackMemberships") + ) { + if (call.query.includes("$headRef: String!")) { + expect(call.query).toContain("headRepository { nameWithOwner viewerPermission }"); + expect(call.query).toContain("headRef { id }"); + } else { + expect(call.query).toContain("headRepository { viewerPermission }"); + } + } + } + }), + ); + + it.effect("preserves the actual source fork name in provider detail", () => + Effect.gen(function* () { + const answer = coreResponse({ + state: "MERGED", + isCrossRepository: true, + headRepository: { nameWithOwner: "contributor/web-fork", viewerPermission: "WRITE" }, + headRef: { id: "REF" }, + }); + mockedExecute.mockReturnValue(Effect.succeed(output(encodeJson(answer)))); + const provider = yield* GitHubPullRequestProvider.make; + const detail = yield* provider.getChangeRequest({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + }); + expect(detail.headRepositoryNameWithOwner).toBe("contributor/web-fork"); + expect(detail.viewerPermissions.actions).toContain("delete-branch"); + expect(queryAt(0)).toContain("headRepository { nameWithOwner viewerPermission }"); + }), + ); + + const branchToDelete = (pullRequest: Readonly> = {}) => ({ + data: { + repository: { + pullRequest: { + state: "MERGED", + headRefName: "feat/delete", + baseRefName: "main", + headRefOid: "abc123", + headRepository: { id: "FORK_REPO" }, + ...pullRequest, + }, + }, + }, + }); + const sourceBranch = (repository: Readonly> = {}) => ({ + data: { + node: { + defaultBranchRef: { name: "main" }, + ref: { target: { oid: "abc123" } }, + ...repository, + }, + }, + }); + + it.effect.each(["MERGED", "CLOSED"])("deletes the exact remote fork branch of a %s PR", (state) => + Effect.gen(function* () { + route( + ["query PullRequestBranchToDelete", branchToDelete({ state })], + ["query PullRequestSourceBranch", sourceBranch()], + ); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + yield* cli.runPullRequestAction({ + cwd: "/w", + repository: "acme/web", + host: "github.enterprise.test", + number: 7, + action: "delete-branch", + }); + expect(variablesOf("query PullRequestBranchToDelete")).toEqual([ + { owner: "acme", name: "web", number: 7 }, + ]); + expect(variablesOf("query PullRequestSourceBranch")).toEqual([ + { repositoryId: "FORK_REPO", ref: "refs/heads/feat/delete" }, + ]); + expect(variablesOf("updateRefs(")).toEqual([ + { + input: { + repositoryId: "FORK_REPO", + refUpdates: [ + { + name: "refs/heads/feat/delete", + beforeOid: "abc123", + afterOid: "0000000000000000000000000000000000000000", + }, + ], + }, + }, + ]); + expect(mockedExecute.mock.calls.at(-1)?.[0].host).toBe("github.enterprise.test"); + expect(variablesOf("mergePullRequest(")).toEqual([]); + expect(variablesOf("closePullRequest(")).toEqual([]); + }), + ); + + it.effect.each([ + { description: "an open PR", head: { state: "OPEN" }, source: {} }, + { description: "a missing source repository", head: { headRepository: null }, source: {} }, + { description: "the base branch", head: { headRefName: "main" }, source: {} }, + { + description: "the default branch", + head: {}, + source: { defaultBranchRef: { name: "feat/delete" } }, + }, + { description: "a missing default branch", head: {}, source: { defaultBranchRef: null } }, + { + description: "a changed branch", + head: {}, + source: { ref: { target: { oid: "new-commit" } } }, + }, + { description: "an unavailable source repository", head: {}, source: null }, + ])("refuses to delete $description", ({ head, source }) => + Effect.gen(function* () { + route( + ["query PullRequestBranchToDelete", branchToDelete(head)], + [ + "query PullRequestSourceBranch", + source === null ? { data: { node: null } } : sourceBranch(source), + ], + ); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const error = yield* Effect.flip( + cli.runPullRequestAction({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + action: "delete-branch", + }), + ); + expect(error._tag).toBe("GitHubBranchDeletionRefusedError"); + expect(variablesOf("updateRefs(")).toEqual([]); + }), + ); + + it.effect("accepts an already deleted source branch", () => + Effect.gen(function* () { + route( + ["query PullRequestBranchToDelete", branchToDelete()], + ["query PullRequestSourceBranch", sourceBranch({ ref: null })], + ); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + yield* cli.runPullRequestAction({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + action: "delete-branch", + }); + expect(variablesOf("updateRefs(")).toEqual([]); + }), + ); + + it.effect("keeps the source branch when GitHub refuses deletion", () => + Effect.gen(function* () { + route( + ["query PullRequestBranchToDelete", branchToDelete()], + ["query PullRequestSourceBranch", sourceBranch()], + ); + const answer = mockedExecute.getMockImplementation()!; + mockedExecute.mockImplementation((call) => + call.kind === "graphql" && call.query.includes("updateRefs(") + ? Effect.fail( + new GitHubApi.GitHubApiResponseError({ + host: call.host, + operation: call.operation, + status: 422, + }), + ) + : answer(call), + ); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const error = yield* Effect.flip( + cli.runPullRequestAction({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + action: "delete-branch", + }), + ); + expect(error._tag).toBe("GitHubApiResponseError"); + expect(variablesOf("mergePullRequest(")).toEqual([]); + expect(variablesOf("closePullRequest(")).toEqual([]); + }), + ); + it.effect.each(["merge", "enable-auto-merge"] as const)( "removes agent credits from the proposed message for %s", (action) => @@ -4103,6 +4347,7 @@ layer("GitHubPullRequestApi.layer", (it) => { expect(access).toEqual({ mergeCapabilities: { merge: true, squash: false, rebase: true }, canWrite: false, + canDeleteBranch: false, canTriage: false, canUpdate: true, didAuthor: true, @@ -4213,6 +4458,7 @@ layer("GitHubPullRequestApi.layer", (it) => { expect(access).toEqual({ mergeCapabilities: { merge: true, squash: false, rebase: true }, canWrite: false, + canDeleteBranch: false, canTriage: false, canUpdate: true, didAuthor: true, diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index b8c7fe2276889e75b018ce79596e1f988e9ce7c3..15ceb701633a0816554a4e0d71ce823e60bcefed 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -311,7 +311,37 @@ export class GitHubWorkflowApprovalHeadChangedError extends Schema.TaggedError()( + "GitHubBranchDeletionRefusedError", + { + number: Schema.Int, + reason: Schema.Literals([ + "open-pr", + "stack", + "source-unavailable", + "unsafe-branch", + "head-changed", + ]), + }, +) { + override get message(): string { + switch (this.reason) { + case "open-pr": + return `Close or merge pull request #${this.number} before deleting its source branch.`; + case "stack": + return "Branch deletion is not supported for a stack."; + case "source-unavailable": + return `GitHub did not report a complete source branch for pull request #${this.number}.`; + case "unsafe-branch": + return "The source branch is a default or base branch and cannot be deleted here."; + case "head-changed": + return "The source branch has new commits and was not deleted."; + } + } +} + export type GitHubPullRequestApiError = + | GitHubBranchDeletionRefusedError | GitHubStackActionError | GitHubApi.GitHubApiError | GitHubPullRequestReadError @@ -1062,6 +1092,60 @@ const MERGE_PULL_REQUEST_GRAPHQL_MUTATION = `mutation($input: MergePullRequestIn const ENABLE_AUTO_MERGE_GRAPHQL_MUTATION = `mutation($input: EnablePullRequestAutoMergeInput!) { enablePullRequestAutoMerge(input: $input) { clientMutationId } }`; + +const BRANCH_TO_DELETE_GRAPHQL_QUERY = `query PullRequestBranchToDelete($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + state + headRefName + baseRefName + headRefOid + headRepository { id } + } + } +}`; +const decodeBranchToDelete = Schema.decodeUnknownResult( + Schema.fromJsonString( + Schema.Struct({ + data: Schema.Struct({ + repository: Schema.Struct({ + pullRequest: Schema.Struct({ + state: Schema.Literals(["OPEN", "CLOSED", "MERGED"]), + headRefName: Schema.String, + baseRefName: Schema.String, + headRefOid: Schema.String, + headRepository: Schema.NullOr(Schema.Struct({ id: Schema.String })), + }), + }), + }), + }), + ), +); +const SOURCE_BRANCH_GRAPHQL_QUERY = `query PullRequestSourceBranch($repositoryId: ID!, $ref: String!) { + node(id: $repositoryId) { + ... on Repository { + defaultBranchRef { name } + ref(qualifiedName: $ref) { target { oid } } + } + } +}`; +const decodeSourceBranch = Schema.decodeUnknownResult( + Schema.fromJsonString( + Schema.Struct({ + data: Schema.Struct({ + node: Schema.NullOr( + Schema.Struct({ + defaultBranchRef: Schema.NullOr(Schema.Struct({ name: Schema.String })), + ref: Schema.NullOr(Schema.Struct({ target: Schema.Struct({ oid: Schema.String }) })), + }), + ), + }), + }), + ), +); +const DELETE_BRANCH_GRAPHQL_MUTATION = `mutation($input: UpdateRefsInput!) { + updateRefs(input: $input) { clientMutationId } +}`; const DISABLE_AUTO_MERGE_GRAPHQL_MUTATION = `mutation($pullRequestId: ID!) { disablePullRequestAutoMerge(input: { pullRequestId: $pullRequestId }) { clientMutationId } }`; @@ -2494,6 +2578,100 @@ export const make = Effect.gen(function* () { }, runPullRequestAction: (input) => { + if (input.action === "delete-branch") { + return Effect.gen(function* () { + if (input.stackNumber !== undefined) { + return yield* new GitHubBranchDeletionRefusedError({ + number: input.number, + reason: "stack", + }); + } + const { owner, name } = parseRepositorySelector(input.repository); + const response = yield* graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "deletePullRequestBranch", + allowReserve: true, + query: BRANCH_TO_DELETE_GRAPHQL_QUERY, + variables: { owner, name, number: input.number }, + decode: decodeBranchToDelete, + }); + const branch = response.data.repository.pullRequest; + if (branch.state === "OPEN") { + return yield* new GitHubBranchDeletionRefusedError({ + number: input.number, + reason: "open-pr", + }); + } + if ( + branch.headRepository === null || + !branch.headRepository.id || + !branch.headRefName || + !branch.headRefOid + ) { + return yield* new GitHubBranchDeletionRefusedError({ + number: input.number, + reason: "source-unavailable", + }); + } + if (branch.headRefName === branch.baseRefName) { + return yield* new GitHubBranchDeletionRefusedError({ + number: input.number, + reason: "unsafe-branch", + }); + } + const ref = `refs/heads/${branch.headRefName}`; + const source = yield* graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "deletePullRequestBranch", + allowReserve: true, + query: SOURCE_BRANCH_GRAPHQL_QUERY, + variables: { repositoryId: branch.headRepository.id, ref }, + decode: decodeSourceBranch, + }); + const repository = source.data.node; + if (repository === null) { + return yield* new GitHubBranchDeletionRefusedError({ + number: input.number, + reason: "source-unavailable", + }); + } + if (repository.ref === null) return; + if ( + repository.defaultBranchRef === null || + branch.headRefName === repository.defaultBranchRef.name + ) { + return yield* new GitHubBranchDeletionRefusedError({ + number: input.number, + reason: "unsafe-branch", + }); + } + if (repository.ref.target.oid !== branch.headRefOid) { + return yield* new GitHubBranchDeletionRefusedError({ + number: input.number, + reason: "head-changed", + }); + } + yield* graphql({ + host: input.host, + operation: "deletePullRequestBranch", + query: DELETE_BRANCH_GRAPHQL_MUTATION, + variables: { + input: { + repositoryId: branch.headRepository.id, + refUpdates: [ + { + name: ref, + beforeOid: branch.headRefOid, + afterOid: "0000000000000000000000000000000000000000", + }, + ], + }, + }, + }); + }); + } if (input.stackNumber !== undefined) return runGitHubStackAction({ ...input, stackNumber: input.stackNumber }).pipe( Effect.provideService(GitHubApi.GitHubApi, api), diff --git a/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts b/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts index d3f7d200426fdb133596258128b9bc6bb55cc37b..e2a155f19cfbaa77bd9a0547098f6ceebbf5aa20 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts @@ -64,6 +64,48 @@ it.effect("maps credential verification failures without relabeling operation fa }), ); +it.effect("advertises standalone branch deletion for source repository writers", () => + Effect.gen(function* () { + let action: string | undefined; + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(GitHubPullRequestApi.GitHubPullRequestApi)({ + runPullRequestAction: (input) => + Effect.sync(() => { + action = input.action; + }), + }), + ), + ); + expect(provider.capabilities.deleteBranch).toBe(true); + expect(provider.capabilities.actions).toContain("delete-branch"); + yield* provider.runAction({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + action: "delete-branch", + }); + expect(action).toBe("delete-branch"); + const access = { + canWrite: false, + canTriage: false, + canUpdate: false, + didAuthor: false, + canDeleteBranch: true, + }; + expect(gitHubViewerPermissions(access).actions).toContain("delete-branch"); + expect( + gitHubViewerPermissions({ + ...access, + canWrite: true, + didAuthor: true, + canDeleteBranch: false, + }).actions, + ).not.toContain("delete-branch"); + }), +); + it.effect("refreshes checks without permissions or comparison reads", () => Effect.gen(function* () { let reads = 0; @@ -219,6 +261,7 @@ describe("gitHubViewerPermissions", () => { expect( gitHubViewerPermissions({ canWrite: true, + canDeleteBranch: true, canTriage: true, canUpdate: true, didAuthor: false, @@ -226,6 +269,7 @@ describe("gitHubViewerPermissions", () => { ).toEqual({ // Arming a merge for later is the merge, so it travels with it. actions: [ + "delete-branch", "merge", "enable-auto-merge", "disable-auto-merge", diff --git a/packages/source-control-github/src/server/GitHubPullRequestProvider.ts b/packages/source-control-github/src/server/GitHubPullRequestProvider.ts index 2d92908f3b1d77e550e9d1eae9798b01a0f2db90..3a99c6d072ba7bd9ace7789a01e96038d75d2e19 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestProvider.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestProvider.ts @@ -27,6 +27,7 @@ const CAPABILITIES: PullRequestCapabilities = { comment: true, actions: [...definition.changeRequestActions], mergeMethods: ["merge", "squash", "rebase"], + deleteBranch: true, updateMethods: ["merge", "rebase"], search: true, reactions: true, @@ -65,6 +66,7 @@ export function gitHubViewerPermissions(access: GitHubViewerAccess): PullRequest return { ...(access.canWrite ? { stackRebase: true } : {}), actions: [ + ...(access.canDeleteBranch === true ? (["delete-branch"] as const) : []), // Arming a merge and taking the arming back are the merge, deferred: whoever may not // merge here may not leave an instruction to merge later either. ...(access.canWrite diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts index 99fe8bd4f8abee19c4c7f923c9e2175a9bf43bd4..ed8febc7e8039969939bee5f0c4a7a2cb8eea192 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.test.ts @@ -1055,6 +1055,7 @@ describe("viewer permission decoding", () => { canTriage: false, canUpdate: true, didAuthor: true, + canDeleteBranch: false, }); }); @@ -1074,6 +1075,7 @@ describe("viewer permission decoding", () => { canTriage: false, canUpdate: false, didAuthor: false, + canDeleteBranch: false, }); }); @@ -1087,6 +1089,7 @@ describe("viewer permission decoding", () => { canTriage: false, canUpdate: true, didAuthor: false, + canDeleteBranch: false, }); }); diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.ts index ea756b5bc54fa0879bb0d299513401aeb066df75..8423ea9a203d345854da496276c7349498f6c080 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.ts @@ -558,6 +558,18 @@ const RawViewerFieldsSchema = Schema.Struct({ viewerDidAuthor: Schema.optional(Schema.Boolean), }); +const RawHeadBranchAccessSchema = Schema.Struct({ + headRepository: Schema.optional( + Schema.NullOr( + Schema.Struct({ + nameWithOwner: Schema.optional(Schema.NullOr(Schema.String)), + viewerPermission: Schema.optional(Schema.NullOr(Schema.String)), + }), + ), + ), + headRef: Schema.optional(Schema.NullOr(Schema.Struct({ id: Schema.String }))), +}); + const RawThreadCommentsSchema = Schema.Struct({ totalCount: Schema.optional(Schema.Int), pageInfo: Schema.optional(RawPageInfoSchema), @@ -722,6 +734,7 @@ const RawCoreSchema = Schema.Struct({ pullRequest: Schema.Struct({ ...RawDetailSchema.fields, ...RawViewerFieldsSchema.fields, + ...RawHeadBranchAccessSchema.fields, viewerCanUpdateBranch: Schema.Boolean, baseRef: Schema.NullOr( Schema.Struct({ @@ -842,6 +855,8 @@ export const pullRequestCoreGraphQlQuery = (host: string) => { additions deletions changedFiles createdAt updatedAt mergedAt closedAt headRefName baseRefName headRefOid isCrossRepository headRepositoryOwner { login } + headRepository { nameWithOwner viewerPermission } + headRef { id } author { login avatarUrl ... on User { id name } } autoMergeRequest { mergeMethod } viewerCanUpdate viewerDidAuthor viewerCanUpdateBranch @@ -1353,6 +1368,7 @@ export interface GitHubPullRequestDetail extends GitHubPullRequestListItem { readonly isCrossRepository?: boolean; /** The owner of the head branch's repository; null where `gh` did not say. */ readonly headRepositoryOwner: string | null; + readonly headRepositoryNameWithOwner?: string | null; readonly headSha?: string | null; readonly body: string; readonly changedFiles: number; @@ -2358,8 +2374,10 @@ export function decodePullRequestCoreJson( labels: pr.labels.nodes, statusCheckRollup: toCheckContexts(contexts?.nodes ?? []), }), + headRepositoryNameWithOwner: trimmed(pr.headRepository?.nameWithOwner), viewerAccess: { canWrite: toCanWrite(repository.viewerPermission), + canDeleteBranch: toCanWrite(pr.headRepository?.viewerPermission) && pr.headRef !== null, canTriage: toCanTriage(repository.viewerPermission), ...toPullRequestViewerFields(pr), mergeCapabilities: { @@ -3181,6 +3199,7 @@ export function buildLabelRequest(labels: ReadonlyArray): { */ export interface GitHubViewerAccess { readonly canWrite: boolean; + readonly canDeleteBranch?: boolean; /** * The viewer's role reaches triage, which is the least that may label. Everyone who can write * can triage; a triager is the one role that can label without being able to merge. @@ -3201,7 +3220,10 @@ export interface GitHubViewerAccess { export const VIEWER_PERMISSIONS_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed viewerPermission - pullRequest(number: $number) { viewerCanUpdate viewerDidAuthor } + pullRequest(number: $number) { + viewerCanUpdate viewerDidAuthor + headRepository { viewerPermission } + } } }`; @@ -3210,7 +3232,9 @@ const RawViewerPermissionsSchema = Schema.Struct({ repository: Schema.Struct({ ...RawRepositoryAccessSchema.fields, /** Null for a number that names no pull request the viewer can see. */ - pullRequest: Schema.NullOr(RawViewerFieldsSchema), + pullRequest: Schema.NullOr( + Schema.Struct({ ...RawViewerFieldsSchema.fields, ...RawHeadBranchAccessSchema.fields }), + ), }), }), }); @@ -3232,6 +3256,7 @@ export function decodeViewerPermissionsJson( rebase: repository.rebaseMergeAllowed, }, canWrite: toCanWrite(repository.viewerPermission), + canDeleteBranch: toCanWrite(repository.pullRequest?.headRepository?.viewerPermission), canTriage: toCanTriage(repository.viewerPermission), ...toPullRequestViewerFields(repository.pullRequest), }); diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts index 643d130c85bc8a35410eec4eadbdb98775c95f4c..7e84236920e272f3b3b09ae68b44b071be82e634 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts @@ -524,6 +524,7 @@ function actionArgs( // Never reached: this host does not declare the action, so the service refuses it first. case "revert": case "approve-workflows": + case "delete-branch": throw new Error(`GitLab merge request action ${action} is unsupported`); } } -- 2.54.0