From a899cd36f65da2ca95f662dd5cb4614978206b2f Mon Sep 17 00:00:00 2001 From: pat-s Date: Tue, 29 Sep 2026 20:53:15 +0200 Subject: [PATCH 1/2] fix(server): respect Forgejo branch deletion settings on merge --- .../ForgejoPullRequestProvider.test.ts | 117 ++++++++++++++++++ .../pullRequest/ForgejoPullRequestProvider.ts | 16 ++- .../src/pullRequest/forgejoPullRequestJson.ts | 1 + 3 files changed, 129 insertions(+), 5 deletions(-) create mode 100644 apps/server/src/pullRequest/ForgejoPullRequestProvider.test.ts diff --git a/apps/server/src/pullRequest/ForgejoPullRequestProvider.test.ts b/apps/server/src/pullRequest/ForgejoPullRequestProvider.test.ts new file mode 100644 index 000000000000..7cfcbd68cf32 --- /dev/null +++ b/apps/server/src/pullRequest/ForgejoPullRequestProvider.test.ts @@ -0,0 +1,117 @@ +import { describe, expect, it } from "@effect/vitest"; +import * as Effect from "effect/Effect"; +import * as Layer from "effect/Layer"; +import { ChildProcessSpawner } from "effect/unstable/process"; +import { ForgejoCli, ForgejoCliError, type ForgejoApiInput } from "../sourceControl/ForgejoCli.ts"; +import { make } from "./ForgejoPullRequestProvider.ts"; + +const reference = { + cwd: "/repo", + repository: "acme/project", + host: "https://forgejo.example.com", + number: 42, +}; +const output = (stdout: string) => ({ + exitCode: ChildProcessSpawner.ExitCode(0), + stdout, + stderr: "", + stdoutTruncated: false, + stderrTruncated: false, +}); + +describe("Forgejo merge branch deletion", () => { + for (const mergeMethod of [undefined, "merge", "squash", "rebase"] as const) { + it.effect.each([true, false, undefined])( + `respects repository deletion setting %s when merging with ${mergeMethod ?? "the default"}`, + (deleteBranch) => + Effect.gen(function* () { + const requests: ForgejoApiInput[] = []; + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(ForgejoCli)({ + api: (input) => { + requests.push(input); + return Effect.succeed( + output( + input.path === "repos/acme/project" + ? JSON.stringify({ + full_name: "acme/project", + default_delete_branch_after_merge: deleteBranch, + }) + : "", + ), + ); + }, + }), + ), + ); + + yield* provider.runAction({ + ...reference, + action: "merge", + ...(mergeMethod === undefined ? {} : { mergeMethod }), + }); + + expect(requests).toHaveLength(2); + expect(requests[0]).toMatchObject({ + ...reference, + path: "repos/acme/project", + }); + expect(requests[0]?.method ?? "GET").toBe("GET"); + expect(requests[1]).toMatchObject({ + ...reference, + path: "repos/acme/project/pulls/42/merge", + method: "POST", + body: { + Do: mergeMethod ?? "merge", + delete_branch_after_merge: deleteBranch ?? false, + }, + }); + }), + ); + } + + it.effect.each(["request failure", "invalid response", "truncated response"] as const)( + "does not merge when reading repository settings fails: %s", + (failure) => + Effect.gen(function* () { + const requests: ForgejoApiInput[] = []; + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(ForgejoCli)({ + api: (input) => { + requests.push(input); + if (failure === "request failure") { + return Effect.fail( + new ForgejoCliError({ + command: "fj", + cwd: reference.cwd, + detail: "Repository settings unavailable", + }), + ); + } + return Effect.succeed({ + ...output( + JSON.stringify({ + full_name: "acme/project", + default_delete_branch_after_merge: + failure === "invalid response" ? "true" : true, + }), + ), + stdoutTruncated: failure === "truncated response", + }); + }, + }), + ), + ); + + const result = yield* Effect.result(provider.runAction({ ...reference, action: "merge" })); + + expect(result).toMatchObject({ + _tag: "Failure", + failure: { _tag: "PullRequestProviderError", provider: "forgejo" }, + }); + expect(requests.map((request) => request.path)).toEqual(["repos/acme/project"]); + }), + ); +}); diff --git a/apps/server/src/pullRequest/ForgejoPullRequestProvider.ts b/apps/server/src/pullRequest/ForgejoPullRequestProvider.ts index fddedbf6059c..a2f6fe67904e 100644 --- a/apps/server/src/pullRequest/ForgejoPullRequestProvider.ts +++ b/apps/server/src/pullRequest/ForgejoPullRequestProvider.ts @@ -413,11 +413,17 @@ export const make = Effect.gen(function* () { runAction: (input) => { switch (input.action) { case "merge": - return write({ - ...input, - path: `${pullPath(input)}/merge`, - method: "POST", - body: { Do: input.mergeMethod ?? "merge" }, + return Effect.gen(function* () { + const repo = yield* getRepo(input); + yield* write({ + ...input, + path: `${pullPath(input)}/merge`, + method: "POST", + body: { + Do: input.mergeMethod ?? "merge", + delete_branch_after_merge: repo.default_delete_branch_after_merge ?? false, + }, + }); }); case "close": case "reopen": diff --git a/apps/server/src/pullRequest/forgejoPullRequestJson.ts b/apps/server/src/pullRequest/forgejoPullRequestJson.ts index 1d0b5c1d4ccd..b1725fe15633 100644 --- a/apps/server/src/pullRequest/forgejoPullRequestJson.ts +++ b/apps/server/src/pullRequest/forgejoPullRequestJson.ts @@ -32,6 +32,7 @@ export const ForgejoRepository = Schema.Struct({ allow_squash_merge: Schema.optional(Schema.Boolean), allow_rebase: Schema.optional(Schema.Boolean), allow_rebase_update: Schema.optional(Schema.Boolean), + default_delete_branch_after_merge: Schema.optional(Schema.Boolean), }); const Branch = Schema.Struct({ ref: Schema.String, From 5aaa1922e8acce4c96dd8302dbc56890ff006764 Mon Sep 17 00:00:00 2001 From: pat-s Date: Thu, 1 Oct 2026 09:27:47 +0200 Subject: [PATCH 2/2] fix(server): report Forgejo merges that land when branch deletion is refused Forgejo deletes the head branch only after the merge lands, and reports a refused deletion (protected branch, fork the merger cannot push to, default branch) as the whole merge request failing with HTTP 403. - Request deletion only when the head branch is deletable by the viewer, as Forgejo's web merge form does: head repository writable and not its default branch. - When a merge that requested deletion fails, re-read the pull request and treat it as merged if Forgejo merged it, logging the kept branch. - Report an unreadable deletion setting as a refused precondition, distinct from the host rejecting the merge. --- .../ForgejoPullRequestProvider.test.ts | 320 ++++++++++++++---- .../pullRequest/ForgejoPullRequestProvider.ts | 49 ++- .../src/pullRequest/forgejoPullRequestJson.ts | 1 + 3 files changed, 295 insertions(+), 75 deletions(-) diff --git a/apps/server/src/pullRequest/ForgejoPullRequestProvider.test.ts b/apps/server/src/pullRequest/ForgejoPullRequestProvider.test.ts index 7cfcbd68cf32..b21afbc007ab 100644 --- a/apps/server/src/pullRequest/ForgejoPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/ForgejoPullRequestProvider.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; +import * as Schema from "effect/Schema"; import { ChildProcessSpawner } from "effect/unstable/process"; import { ForgejoCli, ForgejoCliError, type ForgejoApiInput } from "../sourceControl/ForgejoCli.ts"; import { make } from "./ForgejoPullRequestProvider.ts"; @@ -18,6 +19,92 @@ const output = (stdout: string) => ({ stdoutTruncated: false, stderrTruncated: false, }); +const cliError = (detail: string, httpStatus?: number) => + new ForgejoCliError({ + command: "fj", + cwd: reference.cwd, + detail, + ...(httpStatus === undefined ? {} : { httpStatus }), + ...(httpStatus === 403 ? { reason: "forbidden" as const } : {}), + }); +const repository = (deleteBranch: boolean | undefined) => ({ + full_name: "acme/project", + default_branch: "main", + default_delete_branch_after_merge: deleteBranch, +}); +const headRepository = { full_name: "acme/project", default_branch: "main", push: true }; +const pullRequest = (input: { + readonly merged?: boolean; + readonly ref?: string; + readonly head?: { full_name: string; default_branch: string; push: boolean } | null; +}) => { + const head = input.head === undefined ? headRepository : input.head; + return { + number: reference.number, + title: "Change", + body: null, + html_url: "https://forgejo.example.com/acme/project/pulls/42", + user: { login: "author" }, + state: input.merged ? "closed" : "open", + merged: input.merged ?? false, + head: { + ref: input.ref ?? "feature", + sha: "head", + repo: + head === null + ? null + : { + full_name: head.full_name, + default_branch: head.default_branch, + permissions: { push: head.push, admin: false }, + }, + }, + base: { ref: "main", sha: "base", repo: { full_name: "acme/project" } }, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + closed_at: null, + merged_at: null, + labels: [], + }; +}; +const isForgejoCliError = Schema.is(ForgejoCliError); +const REPO_PATH = "repos/acme/project"; +const PULL_PATH = "repos/acme/project/pulls/42"; +const MERGE_PATH = "repos/acme/project/pulls/42/merge"; + +/** Serves the repository, the pull request, and the merge, recording every request. */ +const forgejo = (input: { + readonly repository?: unknown; + readonly pullRequest?: ReturnType; + readonly afterMerge?: ReturnType | ForgejoCliError; + readonly merge?: ForgejoCliError; +}) => { + const requests: ForgejoApiInput[] = []; + let merged = false; + const layer = Layer.mock(ForgejoCli)({ + api: (request) => { + requests.push(request); + switch (request.path) { + case REPO_PATH: + return Effect.succeed(output(JSON.stringify(input.repository ?? repository(true)))); + case PULL_PATH: { + const pull = merged + ? (input.afterMerge ?? pullRequest({ merged: true })) + : (input.pullRequest ?? pullRequest({})); + return isForgejoCliError(pull) + ? Effect.fail(pull) + : Effect.succeed(output(JSON.stringify(pull))); + } + case MERGE_PATH: + merged = true; + return input.merge ? Effect.fail(input.merge) : Effect.succeed(output("")); + default: + return Effect.die(`unexpected request ${request.path}`); + } + }, + }); + return { requests, layer }; +}; describe("Forgejo merge branch deletion", () => { for (const mergeMethod of [undefined, "merge", "squash", "rebase"] as const) { @@ -25,26 +112,8 @@ describe("Forgejo merge branch deletion", () => { `respects repository deletion setting %s when merging with ${mergeMethod ?? "the default"}`, (deleteBranch) => Effect.gen(function* () { - const requests: ForgejoApiInput[] = []; - const provider = yield* make.pipe( - Effect.provide( - Layer.mock(ForgejoCli)({ - api: (input) => { - requests.push(input); - return Effect.succeed( - output( - input.path === "repos/acme/project" - ? JSON.stringify({ - full_name: "acme/project", - default_delete_branch_after_merge: deleteBranch, - }) - : "", - ), - ); - }, - }), - ), - ); + const host = forgejo({ repository: repository(deleteBranch) }); + const provider = yield* make.pipe(Effect.provide(host.layer)); yield* provider.runAction({ ...reference, @@ -52,15 +121,14 @@ describe("Forgejo merge branch deletion", () => { ...(mergeMethod === undefined ? {} : { mergeMethod }), }); - expect(requests).toHaveLength(2); - expect(requests[0]).toMatchObject({ - ...reference, - path: "repos/acme/project", - }); - expect(requests[0]?.method ?? "GET").toBe("GET"); - expect(requests[1]).toMatchObject({ + expect(host.requests.map((request) => request.path)).toEqual( + deleteBranch ? [REPO_PATH, PULL_PATH, MERGE_PATH] : [REPO_PATH, MERGE_PATH], + ); + expect(host.requests[0]).toMatchObject({ ...reference, path: REPO_PATH }); + expect(host.requests[0]?.method ?? "GET").toBe("GET"); + expect(host.requests.at(-1)).toMatchObject({ ...reference, - path: "repos/acme/project/pulls/42/merge", + path: MERGE_PATH, method: "POST", body: { Do: mergeMethod ?? "merge", @@ -71,47 +139,161 @@ describe("Forgejo merge branch deletion", () => { ); } - it.effect.each(["request failure", "invalid response", "truncated response"] as const)( - "does not merge when reading repository settings fails: %s", - (failure) => - Effect.gen(function* () { - const requests: ForgejoApiInput[] = []; - const provider = yield* make.pipe( - Effect.provide( - Layer.mock(ForgejoCli)({ - api: (input) => { - requests.push(input); - if (failure === "request failure") { - return Effect.fail( - new ForgejoCliError({ - command: "fj", - cwd: reference.cwd, - detail: "Repository settings unavailable", - }), - ); - } - return Effect.succeed({ - ...output( - JSON.stringify({ - full_name: "acme/project", - default_delete_branch_after_merge: - failure === "invalid response" ? "true" : true, - }), - ), - stdoutTruncated: failure === "truncated response", - }); - }, - }), - ), - ); - - const result = yield* Effect.result(provider.runAction({ ...reference, action: "merge" })); - - expect(result).toMatchObject({ - _tag: "Failure", - failure: { _tag: "PullRequestProviderError", provider: "forgejo" }, - }); - expect(requests.map((request) => request.path)).toEqual(["repos/acme/project"]); + it.effect.each([ + { + name: "a fork the viewer cannot push to", + pullRequest: pullRequest({ + head: { full_name: "contributor/project", default_branch: "main", push: false }, + }), + }, + { + name: "the head repository's default branch", + pullRequest: pullRequest({ + ref: "main", + head: { full_name: "contributor/project", default_branch: "main", push: true }, }), + }, + { name: "a deleted head repository", pullRequest: pullRequest({ head: null }) }, + ])("keeps the head branch when it is $name", ({ pullRequest }) => + Effect.gen(function* () { + const host = forgejo({ pullRequest }); + const provider = yield* make.pipe(Effect.provide(host.layer)); + + yield* provider.runAction({ ...reference, action: "merge" }); + + expect(host.requests.at(-1)).toMatchObject({ + path: MERGE_PATH, + body: { Do: "merge", delete_branch_after_merge: false }, + }); + }), + ); + + it.effect("deletes a branch in a fork the viewer can push to", () => + Effect.gen(function* () { + const host = forgejo({ + pullRequest: pullRequest({ + head: { full_name: "contributor/project", default_branch: "main", push: true }, + }), + }); + const provider = yield* make.pipe(Effect.provide(host.layer)); + + yield* provider.runAction({ ...reference, action: "merge" }); + + expect(host.requests.at(-1)).toMatchObject({ + path: MERGE_PATH, + body: { delete_branch_after_merge: true }, + }); + }), + ); + + it.effect("reports a merge that landed when Forgejo refuses to delete the branch", () => + Effect.gen(function* () { + const host = forgejo({ merge: cliError("the head branch is protected", 403) }); + const provider = yield* make.pipe(Effect.provide(host.layer)); + + yield* provider.runAction({ ...reference, action: "merge" }); + + expect(host.requests.map((request) => request.path)).toEqual([ + REPO_PATH, + PULL_PATH, + MERGE_PATH, + PULL_PATH, + ]); + }), + ); + + it.effect.each([ + { name: "the pull request is still open", afterMerge: pullRequest({ merged: false }) }, + { name: "the merge cannot be confirmed", afterMerge: cliError("Unavailable", 503) }, + ])("reports a failed merge when $name", ({ afterMerge }) => + Effect.gen(function* () { + const host = forgejo({ afterMerge, merge: cliError("Merge conflict", 409) }); + const provider = yield* make.pipe(Effect.provide(host.layer)); + + const result = yield* Effect.result(provider.runAction({ ...reference, action: "merge" })); + + expect(result).toMatchObject({ + _tag: "Failure", + failure: { + _tag: "PullRequestProviderError", + operation: MERGE_PATH, + detail: "Merge conflict", + }, + }); + expect(host.requests.map((request) => request.path)).toEqual([ + REPO_PATH, + PULL_PATH, + MERGE_PATH, + PULL_PATH, + ]); + }), + ); + + it.effect("does not reread a failed merge that never asked to delete the branch", () => + Effect.gen(function* () { + const host = forgejo({ + repository: repository(false), + merge: cliError("Merge conflict", 409), + }); + const provider = yield* make.pipe(Effect.provide(host.layer)); + + const result = yield* Effect.result(provider.runAction({ ...reference, action: "merge" })); + + expect(result).toMatchObject({ _tag: "Failure", failure: { detail: "Merge conflict" } }); + expect(host.requests.map((request) => request.path)).toEqual([REPO_PATH, MERGE_PATH]); + }), + ); + + it.effect.each([ + "repository request failure", + "invalid repository", + "truncated repository", + "pull request request failure", + ] as const)("does not merge when reading branch deletion settings fails: %s", (failure) => + Effect.gen(function* () { + const requests: ForgejoApiInput[] = []; + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(ForgejoCli)({ + api: (input) => { + requests.push(input); + if ( + failure === + (input.path === REPO_PATH + ? "repository request failure" + : "pull request request failure") + ) { + return Effect.fail(cliError("Repository settings unavailable")); + } + return Effect.succeed({ + ...output( + JSON.stringify( + input.path === REPO_PATH + ? { + ...repository(true), + default_delete_branch_after_merge: + failure === "invalid repository" ? "true" : true, + } + : pullRequest({}), + ), + ), + stdoutTruncated: failure === "truncated repository", + }); + }, + }), + ), + ); + + const result = yield* Effect.result(provider.runAction({ ...reference, action: "merge" })); + + expect(result).toMatchObject({ + _tag: "Failure", + failure: { _tag: "PullRequestProviderError", provider: "forgejo", operation: "merge" }, + }); + expect(result._tag === "Failure" && result.failure.detail).toMatch( + /^The pull request was not merged because its branch deletion setting could not be read\./, + ); + expect(requests.map((request) => request.path)).not.toContain(MERGE_PATH); + }), ); }); diff --git a/apps/server/src/pullRequest/ForgejoPullRequestProvider.ts b/apps/server/src/pullRequest/ForgejoPullRequestProvider.ts index a2f6fe67904e..609b1c8b9b64 100644 --- a/apps/server/src/pullRequest/ForgejoPullRequestProvider.ts +++ b/apps/server/src/pullRequest/ForgejoPullRequestProvider.ts @@ -62,6 +62,11 @@ const issuePath = (input: ProviderRepositoryRef & { readonly number: number }) = // Review IDs differ from the issue-comment IDs used by Forgejo's reactions API. const reviewCommentId = (review: typeof ForgejoReview.Type) => /#issuecomment-([1-9]\d*)$/.exec(review.html_url ?? "")?.[1]; +// Forgejo's web merge form offers to delete the head branch only when the viewer can: not in a +// fork they cannot push to, and never a default branch. Branch protection is not on the pull +// request, so a protected branch is left to the merge's own refusal. +const headBranchDeletable = (pr: typeof ForgejoPullRequest.Type) => + pr.head.repo?.permissions?.push === true && pr.head.ref !== pr.head.repo.default_branch; export const make = Effect.gen(function* () { const cli = yield* ForgejoCli; @@ -414,16 +419,48 @@ export const make = Effect.gen(function* () { switch (input.action) { case "merge": return Effect.gen(function* () { - const repo = yield* getRepo(input); + // Forgejo's merge endpoint ignores the repository's deletion default, so it is read + // first. An unreadable setting must not count as disabled, and must not read as the + // host refusing the merge either. + const settingsUnavailable = (error: PullRequestProviderError) => + new PullRequestProviderError({ + provider: "forgejo", + operation: "merge", + reason: error.reason, + detail: `The pull request was not merged because its branch deletion setting could not be read. ${error.detail}`, + ...(error.retryAt === undefined ? {} : { retryAt: error.retryAt }), + cause: error, + }); + const repo = yield* getRepo(input).pipe(Effect.mapError(settingsUnavailable)); + const pr = repo.default_delete_branch_after_merge + ? yield* getPull(input).pipe(Effect.mapError(settingsUnavailable)) + : null; + const deleteBranch = pr !== null && headBranchDeletable(pr); yield* write({ ...input, path: `${pullPath(input)}/merge`, method: "POST", - body: { - Do: input.mergeMethod ?? "merge", - delete_branch_after_merge: repo.default_delete_branch_after_merge ?? false, - }, - }); + body: { Do: input.mergeMethod ?? "merge", delete_branch_after_merge: deleteBranch }, + }).pipe( + // Forgejo deletes the branch only after the merge has landed, and reports a refused + // deletion (a protected branch, say) as the whole request failing. + Effect.catch((error) => + deleteBranch + ? getPull(input).pipe( + Effect.matchEffect({ + onFailure: () => Effect.fail(error), + onSuccess: (merged) => + merged.merged + ? Effect.logWarning( + "Forgejo merged the pull request but kept its head branch", + { detail: error.detail }, + ) + : Effect.fail(error), + }), + ) + : Effect.fail(error), + ), + ); }); case "close": case "reopen": diff --git a/apps/server/src/pullRequest/forgejoPullRequestJson.ts b/apps/server/src/pullRequest/forgejoPullRequestJson.ts index b1725fe15633..df19830c2227 100644 --- a/apps/server/src/pullRequest/forgejoPullRequestJson.ts +++ b/apps/server/src/pullRequest/forgejoPullRequestJson.ts @@ -26,6 +26,7 @@ export const ForgejoLabel = Schema.Struct({ }); export const ForgejoRepository = Schema.Struct({ full_name: Schema.String, + default_branch: Schema.optional(Schema.String), permissions: Schema.optional(Schema.Struct({ push: Schema.Boolean, admin: Schema.Boolean })), archived: Schema.optional(Schema.Boolean), allow_merge_commits: Schema.optional(Schema.Boolean),