From bdf0b2eb2cf960c153e47885960f299f1b182688 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 22 Aug 2026 15:45:32 -0400 Subject: [PATCH 1/5] feat(pull-requests): say why a merge was refused, and let an administrator merge anyway A pull request held back by its base branch's rules is a normal end to a day's work, and the host's own explanation was being dropped one layer below the toast. Every refusal read as the same guess about write access, which leaves the reader pressing the same button again. Someone who administers the repository already has the authority to merge past those rules, and had to leave the app for a terminal to use it. Signed-off-by: Yordis Prieto --- .../pullRequest/GitHubPullRequestCli.test.ts | 51 ++++++++++- .../src/pullRequest/GitHubPullRequestCli.ts | 9 +- .../GitHubPullRequestProvider.test.ts | 61 +++++++++++-- .../pullRequest/GitHubPullRequestProvider.ts | 6 ++ .../src/pullRequest/PullRequestProvider.ts | 5 ++ .../pullRequest/PullRequestService.test.ts | 89 +++++++++++++++++++ .../src/pullRequest/PullRequestService.ts | 39 ++++++++ .../pullRequest/gitHubPullRequestJson.test.ts | 29 +++++- .../src/pullRequest/gitHubPullRequestJson.ts | 12 +++ .../src/sourceControl/GitHubCli.test.ts | 20 +++++ apps/server/src/sourceControl/GitHubCli.ts | 27 ++++++ apps/server/src/sourceControl/GitLabCli.ts | 5 ++ apps/server/src/vcs/VcsProcess.test.ts | 75 ++++++++++++++++ apps/server/src/vcs/VcsProcess.ts | 18 ++++ .../pullRequest/PullRequestDetailPanel.tsx | 85 ++++++++++++++---- .../pullRequest/pullRequestDetail.logic.ts | 14 +-- packages/contracts/src/pullRequest.ts | 18 ++++ packages/contracts/src/vcs.ts | 51 ++++++++--- 18 files changed, 564 insertions(+), 50 deletions(-) diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts index e114abc180cb..b5554730fffc 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -1089,6 +1089,43 @@ layer("GitHubPullRequestCli.layer", (it) => { }), ); + it.effect("stands the branch's rules down only when asked to", () => + Effect.gen(function* () { + mockedExecute.mockReturnValue(Effect.succeed(output(""))); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + yield* cli.runPullRequestAction({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + action: "merge", + mergeMethod: "squash", + bypassRules: true, + }); + expect(callAt(0).args).toEqual([ + "pr", + "merge", + "7", + "--repo", + "github.com/acme/web", + "--squash", + "--admin", + ]); + + yield* cli.runPullRequestAction({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + action: "merge", + mergeMethod: "squash", + bypassRules: false, + }); + expect(callAt(1).args).not.toContain("--admin"); + }), + ); + it.effect("arms auto-merge with the same strategy a merge would have used", () => Effect.gen(function* () { mockedExecute.mockReturnValue(Effect.succeed(output(""))); @@ -2300,7 +2337,12 @@ layer("GitHubPullRequestCli.layer", (it) => { // One request, because both answers hang off the same repository object. assert.strictEqual(mockedExecute.mock.calls.length, 1); expect(callAt(0).args).toContain("number=7"); - expect(access).toEqual({ canWrite: false, canUpdate: true, didAuthor: true }); + expect(access).toEqual({ + canWrite: false, + canAdminister: false, + canUpdate: true, + didAuthor: true, + }); }), ); @@ -2463,7 +2505,12 @@ layer("GitHubPullRequestCli.layer", (it) => { }); assert.strictEqual(mockedExecute.mock.calls.length, 2); - expect(access).toEqual({ canWrite: false, canUpdate: true, didAuthor: true }); + expect(access).toEqual({ + canWrite: false, + canAdminister: false, + canUpdate: true, + didAuthor: true, + }); yield* TestClock.setTime(Date.parse("2100-01-01T00:00:00Z")); }), ); diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index 73b9d29005dd..8b55695e9bb1 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -525,6 +525,8 @@ export class GitHubPullRequestCli extends Context.Service< readonly action: PullRequestAction; readonly mergeMethod?: PullRequestMergeMethod; readonly updateMethod?: PullRequestUpdateMethod; + /** Only read for `merge`: gh has no bypass for anything else, and refuses it with `--auto`. */ + readonly bypassRules?: boolean; }) => Effect.Effect; readonly commentOnPullRequest: (input: { @@ -845,10 +847,14 @@ function actionArgs( action: PullRequestAction, mergeMethod: PullRequestMergeMethod | undefined, updateMethod: PullRequestUpdateMethod | undefined, + bypassRules: boolean | undefined, ): ReadonlyArray { switch (action) { + // `--admin` is gh's name for merging with the repository's rules stood down. It is refused + // by GitHub itself for anyone the repository does not allow that, so it is asked for only + // where the viewer's permissions already said yes. case "merge": - return ["merge", `--${mergeMethod ?? "merge"}`]; + return ["merge", `--${mergeMethod ?? "merge"}`, ...(bypassRules === true ? ["--admin"] : [])]; // `--auto` arms the same command instead of running it, and still needs the strategy: GitHub // stores the strategy with the standing instruction rather than choosing one at merge time. case "enable-auto-merge": @@ -1752,6 +1758,7 @@ export const make = Effect.gen(function* () { input.action, input.mergeMethod, input.updateMethod, + input.bypassRules, ); return github .execute({ diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index 2555c05dc8fc..8bb058d62de9 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -8,8 +8,15 @@ import { gitHubViewerPermissions, loginAvatarUrl, make } from "./GitHubPullReque import type { GitHubReviewThreadComments } from "./gitHubPullRequestJson.ts"; describe("gitHubViewerPermissions", () => { - it("offers everything to a viewer who can write to the repository", () => { - expect(gitHubViewerPermissions({ canWrite: true, canUpdate: true, didAuthor: false })).toEqual({ + it("offers everything to a viewer who administers the repository", () => { + expect( + gitHubViewerPermissions({ + canWrite: true, + canUpdate: true, + didAuthor: false, + canAdminister: true, + }), + ).toEqual({ // Arming a merge for later is the merge, so it travels with it. actions: [ "merge", @@ -24,14 +31,31 @@ describe("gitHubViewerPermissions", () => { resolve: true, verdicts: ["comment", "approve", "request-changes"], requestReviewers: true, + mergeBypass: true, }); }); + it("keeps merging past the branch's rules to the administrators who may", () => { + expect( + gitHubViewerPermissions({ + canWrite: true, + canUpdate: true, + didAuthor: false, + canAdminister: false, + }).mergeBypass, + ).toBe(false); + }); + it("leaves a passer-by on a repository they can only read nothing but the review", () => { // Every open-source pull request somebody else opened: GitHub says no to all five actions // and to resolving, and yes to commenting and to every verdict. expect( - gitHubViewerPermissions({ canWrite: false, canUpdate: false, didAuthor: false }), + gitHubViewerPermissions({ + canWrite: false, + canUpdate: false, + didAuthor: false, + canAdminister: false, + }), ).toEqual({ actions: [], comment: true, @@ -39,11 +63,19 @@ describe("gitHubViewerPermissions", () => { verdicts: ["comment", "approve", "request-changes"], // Asking somebody else to review is the one thing read access never stretches to. requestReviewers: false, + mergeBypass: false, }); }); it("keeps an author's own pull request theirs to close, with read access and no more", () => { - expect(gitHubViewerPermissions({ canWrite: false, canUpdate: true, didAuthor: true })).toEqual({ + expect( + gitHubViewerPermissions({ + canWrite: false, + canUpdate: true, + didAuthor: true, + canAdminister: false, + }), + ).toEqual({ // Merging is the one thing writing is needed for, now or later; the rest an author may do. actions: ["ready", "draft", "close", "reopen"], comment: true, @@ -51,6 +83,7 @@ describe("gitHubViewerPermissions", () => { // GitHub refuses an author's approval of their own change, so the page does not offer one. verdicts: ["comment"], requestReviewers: false, + mergeBypass: false, }); }); @@ -70,6 +103,7 @@ describe("gitHubViewerPermissions", () => { resolve: false, verdicts: ["comment", "approve", "request-changes"], requestReviewers: false, + mergeBypass: false, }); }).pipe( Effect.provide( @@ -110,7 +144,12 @@ describe("gitHubViewerPermissions", () => { mergeCapabilities: { merge: true, squash: true, rebase: true }, }), getViewerAccess: () => - Effect.succeed({ canWrite: false, canUpdate: true, didAuthor: false }), + Effect.succeed({ + canWrite: false, + canUpdate: true, + didAuthor: false, + canAdminister: false, + }), }), ), ), @@ -157,7 +196,8 @@ describe("getViewerPermissions", () => { Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({ getPullRequestDetail: () => Effect.succeed(openDetail), getPullRequestBaseComparison: () => comparison, - getViewerAccess: () => Effect.succeed({ canWrite: true, canUpdate: true, didAuthor: false }), + getViewerAccess: () => + Effect.succeed({ canWrite: true, canUpdate: true, didAuthor: false, canAdminister: false }), }); it.effect("offers update-branch when the comparison grants it", () => @@ -203,7 +243,7 @@ describe("getViewerPermissions", () => { getViewerAccess: (input) => Effect.sync(() => { viewerAllowReserve = input.allowReserve; - return { canWrite: true, canUpdate: true, didAuthor: false }; + return { canWrite: true, canUpdate: true, didAuthor: false, canAdminister: false }; }), }), ), @@ -238,7 +278,12 @@ describe("getViewerPermissions", () => { }), ), getViewerAccess: () => - Effect.succeed({ canWrite: true, canUpdate: true, didAuthor: false }), + Effect.succeed({ + canWrite: true, + canUpdate: true, + didAuthor: false, + canAdminister: false, + }), }), ), ), diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index ae057251fca9..718bb7bc2537 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -30,6 +30,7 @@ const CAPABILITIES: PullRequestCapabilities = { "disable-auto-merge", ], mergeMethods: ["merge", "squash", "rebase"], + mergeBypass: true, updateMethods: ["merge", "rebase"], search: true, reactions: true, @@ -79,6 +80,10 @@ export function gitHubViewerPermissions(access: GitHubViewerAccess): PullRequest // leaves them commenting, which is what an author has to say about their own change anyway. verdicts: access.didAuthor ? (["comment"] as const) : CAPABILITIES.review.verdicts, requestReviewers: access.canWrite, + // Only an administrator, and only where the plain merge is already theirs: a bypass is that + // same merge with the repository's rules stood down, not a way into a repository this + // account may not write to at all. + mergeBypass: access.canWrite && access.canAdminister, ...(access.canUpdateBranch === true ? { updateMethods: CAPABILITIES.updateMethods } : {}), }; } @@ -431,6 +436,7 @@ export const make = Effect.gen(function* () { action: input.action, ...(input.mergeMethod === undefined ? {} : { mergeMethod: input.mergeMethod }), ...(input.updateMethod === undefined ? {} : { updateMethod: input.updateMethod }), + ...(input.bypassRules === undefined ? {} : { bypassRules: input.bypassRules }), }) .pipe(Effect.mapError(fail("runAction"))), diff --git a/apps/server/src/pullRequest/PullRequestProvider.ts b/apps/server/src/pullRequest/PullRequestProvider.ts index 1ecba8c04224..15e07fd7e788 100644 --- a/apps/server/src/pullRequest/PullRequestProvider.ts +++ b/apps/server/src/pullRequest/PullRequestProvider.ts @@ -393,6 +393,11 @@ export interface PullRequestProviderApi { readonly mergeMethod?: PullRequestMergeMethod; /** Only meaningful for `update-branch`; absent takes the host's own default. */ readonly updateMethod?: PullRequestUpdateMethod; + /** + * Merge with the branch's rules stood down. Only meaningful for `merge`, and only ever + * passed to a provider that reports `capabilities.mergeBypass`. + */ + readonly bypassRules?: boolean; }, ) => Effect.Effect; diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 987dba0d1bde..9ce84ab466b5 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -1930,6 +1930,95 @@ it.effect("refuses a merge strategy the host does not offer", () => }), ); +it.effect("only lets a merge past the branch's rules when host and account both allow it", () => + Effect.gen(function* () { + const bypasses: Array = []; + const capabilities = { + diff: true, + comment: true, + actions: ["merge", "close"] as const, + mergeMethods: ["merge", "squash"] as const, + search: true, + reactions: true, + review: FULL_REVIEW, + reviewers: FULL_REVIEWERS, + }; + const viewerPermissions = { + actions: ["merge", "close"] as const, + comment: true, + resolve: true, + verdicts: ["comment", "approve", "request-changes"] as const, + requestReviewers: true, + }; + const reference = { + projectId: "p1" as ProjectId, + repository: "pingdotgg/t3code", + number: 1, + }; + const serviceFor = (provider: Partial) => + makeService({ + projects: [ + project({ + id: "p1", + title: "t3code", + workspaceRoot: "/a", + repository: "pingdotgg/t3code", + }), + ], + providers: [ + fakeProvider("github", { + runAction: (input) => { + bypasses.push(input.bypassRules); + return Effect.void; + }, + ...provider, + }), + ], + }); + + // A host with no override of its own: Bitbucket and Azure DevOps never report one. + const noOverride = yield* serviceFor({ + capabilities, + getViewerPermissions: () => Effect.succeed(viewerPermissions), + }); + assert.strictEqual( + (yield* Effect.flip( + noOverride.runAction({ ...reference, action: "merge", bypassRules: true }), + ))._tag, + "PullRequestOperationError", + ); + + // The host allows it, this account does not, which is every non-administrator on GitHub. + const notAnAdmin = yield* serviceFor({ + capabilities: { ...capabilities, mergeBypass: true }, + getViewerPermissions: () => Effect.succeed(viewerPermissions), + }); + assert.strictEqual( + (yield* Effect.flip( + notAnAdmin.runAction({ ...reference, action: "merge", bypassRules: true }), + ))._tag, + "PullRequestOperationError", + ); + assert.deepStrictEqual(bypasses, []); + + const administrator = yield* serviceFor({ + capabilities: { ...capabilities, mergeBypass: true }, + getViewerPermissions: () => Effect.succeed({ ...viewerPermissions, mergeBypass: true }), + }); + // Arming a merge for later cannot stand the rules down: it is those very rules it waits on. + assert.strictEqual( + (yield* Effect.flip( + administrator.runAction({ ...reference, action: "close", bypassRules: true }), + ))._tag, + "PullRequestOperationError", + ); + + yield* administrator.runAction({ ...reference, action: "merge", bypassRules: true }); + yield* administrator.runAction({ ...reference, action: "merge" }); + assert.deepStrictEqual(bypasses, [true, undefined]); + }), +); + it.effect("hands the provider the host its repository lives on", () => Effect.gen(function* () { const hosts: string[] = []; diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 41b2a5bd61c5..f7f105efd809 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -199,6 +199,14 @@ const VERDICT_LABELS: Record = { * refusal the host would have answered with. Merging is the one that needs write and nothing * else; the other four are also the author's to take, whatever access they have. */ +/** + * Said instead of the plain merge refusal when the override was the thing asked for: someone + * with write access is told what they are missing rather than that they cannot merge, which + * they can. + */ +const MERGE_BYPASS_ACCESS_REFUSAL = + "You need admin access on this repository to merge past its own rules."; + const ACTION_ACCESS_REFUSALS: Record = { merge: "You need write access on this repository to merge.", ready: @@ -1390,6 +1398,28 @@ export const make = Effect.gen(function* () { }), ); } + // A bypass is asked for by name, so it is refused by name as well. Two ways to get it + // wrong: a host with no override at all, and an override asked for alongside an action + // that is not the merge. gh refuses `--admin` with `--auto`, and the deferred merge + // waits on the very rules a bypass would stand down. + if (input.bypassRules === true) { + if (project.api.capabilities.mergeBypass !== true) { + return Effect.fail( + new PullRequestOperationError({ + operation: "runAction", + detail: "This host cannot merge a change request past its own rules.", + }), + ); + } + if (input.action !== "merge") { + return Effect.fail( + new PullRequestOperationError({ + operation: "runAction", + detail: `A ${input.action} cannot be taken past the branch's rules.`, + }), + ); + } + } // The same for the way a stale branch is brought up to date: a host that only merges // must not be asked to rebase and left to pick something else. if ( @@ -1427,6 +1457,14 @@ export const make = Effect.gen(function* () { }), ); } + if (input.bypassRules === true && viewer.mergeBypass !== true) { + return Effect.fail( + new PullRequestOperationError({ + operation: "runAction", + detail: MERGE_BYPASS_ACCESS_REFUSAL, + }), + ); + } return project.api .runAction({ cwd: project.project.workspaceRoot, @@ -1436,6 +1474,7 @@ export const make = Effect.gen(function* () { action: input.action, ...(input.mergeMethod === undefined ? {} : { mergeMethod: input.mergeMethod }), ...(input.updateMethod === undefined ? {} : { updateMethod: input.updateMethod }), + ...(input.bypassRules === undefined ? {} : { bypassRules: input.bypassRules }), }) .pipe(Effect.mapError(toPullRequestError("runAction"))); }), diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts index f20f20d5a265..e2836f4ffa1c 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts @@ -765,7 +765,31 @@ describe("viewer permission decoding", () => { }), ), ), - ).toEqual({ canWrite: false, canUpdate: true, didAuthor: true }); + ).toEqual({ canWrite: false, canAdminister: false, canUpdate: true, didAuthor: true }); + }); + + it("counts only an administrator as able to merge past the branch's own rules", () => { + expect( + expectSuccess( + decodeViewerPermissionsJson( + viewerJson({ + viewerPermission: "ADMIN", + pullRequest: { viewerCanUpdate: true, viewerDidAuthor: false }, + }), + ), + ), + ).toEqual({ canWrite: true, canAdminister: true, canUpdate: true, didAuthor: false }); + + expect( + expectSuccess( + decodeViewerPermissionsJson( + viewerJson({ + viewerPermission: "WRITE", + pullRequest: { viewerCanUpdate: true, viewerDidAuthor: false }, + }), + ), + ).canAdminister, + ).toBe(false); }); it("says no to a passer-by on a repository they can only read", () => { @@ -778,7 +802,7 @@ describe("viewer permission decoding", () => { }), ), ), - ).toEqual({ canWrite: false, canUpdate: false, didAuthor: false }); + ).toEqual({ canWrite: false, canAdminister: false, canUpdate: false, didAuthor: false }); }); it("reads silence as permission, but not as authorship", () => { @@ -787,6 +811,7 @@ describe("viewer permission decoding", () => { // and claiming it for someone who did not is how an author's own rules get handed out. expect(expectSuccess(decodeViewerPermissionsJson(viewerJson({ pullRequest: null })))).toEqual({ canWrite: false, + canAdminister: false, canUpdate: true, didAuthor: false, }); diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.ts index 773b3aa6700b..09a6759eaf13 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.ts @@ -1891,6 +1891,15 @@ function toCanWrite(viewerPermission: string | null | undefined): boolean { } } +/** + * Whether the viewer administers the repository, which is the only role GitHub offers a merge + * that overrides the rules the repository set for itself. MAINTAIN is not one: it runs the + * project day to day and is still held to those rules. + */ +function toCanAdminister(viewerPermission: string | null | undefined): boolean { + return viewerPermission?.trim().toUpperCase() === "ADMIN"; +} + export function decodeRepositoryAccessJson( raw: string, ): Result.Result { @@ -2126,6 +2135,8 @@ export function buildReviewerRequestJson( */ export interface GitHubViewerAccess { readonly canWrite: boolean; + /** An ADMIN role on the repository, which is the one GitHub lets override its branch rules. */ + readonly canAdminister: boolean; /** GitHub's own `viewerCanUpdate`, true for the author as well as for anyone with write. */ readonly canUpdate: boolean; readonly didAuthor: boolean; @@ -2172,6 +2183,7 @@ export function decodeViewerPermissionsJson( const repository = decoded.success.data.repository; return Result.succeed({ canWrite: toCanWrite(repository.viewerPermission), + canAdminister: toCanAdminister(repository.viewerPermission), ...toPullRequestViewerFields(repository.pullRequest), }); } diff --git a/apps/server/src/sourceControl/GitHubCli.test.ts b/apps/server/src/sourceControl/GitHubCli.test.ts index 964ed3d021c1..9302d4eb4bbd 100644 --- a/apps/server/src/sourceControl/GitHubCli.test.ts +++ b/apps/server/src/sourceControl/GitHubCli.test.ts @@ -52,6 +52,26 @@ describe("GitHubCli.layer", () => { assert.notProperty(commandFailure, "operation"); }); + it("carries the reason a merge was refused instead of a bare command failure", () => { + const context = { command: "gh", cwd: "/repo" } as const; + const refusal = new VcsProcessExitError({ + operation: "GitHubCli.execute", + command: "gh", + cwd: context.cwd, + exitCode: 1, + failureKind: "merge-blocked", + detail: "The target branch's rules do not allow this merge yet.", + stderrLength: 214, + stderrTruncated: false, + }); + + const error = GitHubCli.fromVcsError(context, refusal); + + assert.strictEqual(error._tag, "GitHubCliRefusedError"); + assert.strictEqual(error.detail, refusal.detail); + assert.strictEqual(error.cause, refusal); + }); + it.effect("parses pull request view output", () => Effect.gen(function* () { mockRun.mockReturnValueOnce( diff --git a/apps/server/src/sourceControl/GitHubCli.ts b/apps/server/src/sourceControl/GitHubCli.ts index 974574cbd20e..5b5ccbebae54 100644 --- a/apps/server/src/sourceControl/GitHubCli.ts +++ b/apps/server/src/sourceControl/GitHubCli.ts @@ -77,6 +77,22 @@ export class GitHubPullRequestNotFoundError extends Schema.TaggedErrorClass()( + "GitHubCliRefusedError", + { ...gitHubCliFailureFields, detail: Schema.String }, +) { + override get message(): string { + return `GitHub CLI failed in execute: ${this.detail}`; + } +} + export class GitHubCliCommandError extends Schema.TaggedErrorClass()( "GitHubCliCommandError", gitHubCliFailureFields, @@ -153,6 +169,7 @@ export const GitHubCliError = Schema.Union([ GitHubCliAuthenticationError, GitHubCliRateLimitError, GitHubPullRequestNotFoundError, + GitHubCliRefusedError, GitHubCliCommandError, GitHubPullRequestListDecodeError, GitHubChangeRequestListDecodeError, @@ -190,6 +207,16 @@ export function fromVcsError( if (error.failureKind === "not-found") { return new GitHubPullRequestNotFoundError({ ...context, cause: error }); } + // The classifier already wrote the sentence for the kinds it recognises, so the refusal is + // carried rather than restated. Everything it did not recognise stays a bare command failure: + // an invented reason is worse than none. + if ( + error.failureKind === "merge-blocked" || + error.failureKind === "merge-conflict" || + error.failureKind === "already-merged" + ) { + return new GitHubCliRefusedError({ ...context, detail: error.detail, cause: error }); + } } return new GitHubCliCommandError({ ...context, cause: error }); diff --git a/apps/server/src/sourceControl/GitLabCli.ts b/apps/server/src/sourceControl/GitLabCli.ts index 9a9fc3360247..0040740d8a0f 100644 --- a/apps/server/src/sourceControl/GitLabCli.ts +++ b/apps/server/src/sourceControl/GitLabCli.ts @@ -141,6 +141,11 @@ export class GitLabCliCommandError extends Schema.TaggedErrorClass { }).pipe(provideLive), ); + const exitWith = (command: string, stderr: string) => + VcsProcess.make.pipe( + Effect.provideService( + ProcessRunner.ProcessRunner, + ProcessRunner.ProcessRunner.of({ + run: () => + Effect.succeed({ + stdout: "", + stderr, + code: ChildProcessSpawner.ExitCode(1), + timedOut: false, + stdoutTruncated: false, + stderrTruncated: false, + stdoutInvalidUtf8: false, + stderrInvalidUtf8: false, + }), + }), + ), + Effect.flatMap((service) => service.run({ ...baseInput, command })), + Effect.flip, + ); + + it.effect("says the branch's rules refused the merge, in this repository's own words", () => + Effect.gen(function* () { + const providerStderr = + "X Pull request acme/web#7 is not mergeable: the base branch policy prohibits the merge.\nTo have the pull request merged after all the requirements have been met, add the `--auto` flag.\n"; + const error = yield* exitWith("gh", providerStderr); + + expect(error).toMatchObject({ + detail: "The target branch's rules do not allow this merge yet.", + failureKind: "merge-blocked", + }); + expect(error.message).not.toContain(providerStderr); + }), + ); + + it.effect( + "separates a conflict from a rule, since only one of them is the branch's contents", + () => + Effect.gen(function* () { + const error = yield* exitWith( + "gh", + "X Pull request acme/web#7 is not mergeable: the merge commit cannot be cleanly created.\n", + ); + + expect(error).toMatchObject({ + detail: "The two branches conflict, so no merge commit can be created from them.", + failureKind: "merge-conflict", + }); + }), + ); + + it.effect("recognises a pull request somebody else already merged", () => + Effect.gen(function* () { + const error = yield* exitWith("gh", "X Pull request acme/web#7 was already merged\n"); + + expect(error).toMatchObject({ + detail: "The pull request has already been merged.", + failureKind: "already-merged", + }); + }), + ); + + it.effect("leaves other tools the generic reason rather than guessing at their wording", () => + Effect.gen(function* () { + const error = yield* exitWith("glab", "the merge commit cannot be cleanly created"); + + expect(error).toMatchObject({ + detail: "Process exited with a non-zero status.", + failureKind: "command-failed", + }); + }), + ); + it.effect("retains spawn causes without exposing process arguments in the error message", () => Effect.gen(function* () { const secretArgument = "--token=super-secret-token"; diff --git a/apps/server/src/vcs/VcsProcess.ts b/apps/server/src/vcs/VcsProcess.ts index ec245fa13604..6d000d14893a 100644 --- a/apps/server/src/vcs/VcsProcess.ts +++ b/apps/server/src/vcs/VcsProcess.ts @@ -96,6 +96,24 @@ const classifyNonZeroExit = (command: string, stderr: string): VcsProcessExitFai return "not-found"; } + // Why a merge was refused, which is the one failure here a reader can do something about. + // Matched on the host's own wording rather than passed through: the sentence a reader sees is + // written in the contract, and only the reason travels. gh's phrases alone so far, since the rest + // would be guesses, and a wrong reason is worse than the generic one. + if (command === "gh") { + if (normalized.includes("already merged")) { + return "already-merged"; + } + if (normalized.includes("the merge commit cannot be cleanly created")) { + return "merge-conflict"; + } + // Every other refusal gh reports comes through one shape, carrying a reason this does not + // have to enumerate: a branch the host will not merge yet, whatever gh's name for it is. + if (normalized.includes("is not mergeable")) { + return "merge-blocked"; + } + } + return "command-failed"; }; diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 731a3acecef4..81bd5418c544 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -34,6 +34,7 @@ import { PencilIcon, RefreshCwIcon, ServerIcon, + ShieldOffIcon, TriangleAlertIcon, } from "lucide-react"; import { @@ -192,6 +193,10 @@ const ACTION_FAILURE_HINTS: Record = { "The host refused it. Check that you have write access, and that the merge has not already happened.", }; +/** Said when the override itself was refused, which is a different dead end from a plain merge. */ +const MERGE_BYPASS_FAILURE_HINT = + "The host refused it even with the rules stood down. A repository can forbid its own administrators from bypassing, and nothing bypasses a branch that conflicts."; + /** * Said instead of the update hint when the reader asked for a rebase: it is the one that fails on * its own merits, because GitHub replays the commits and stops at the first that does not apply. @@ -456,7 +461,7 @@ export function PullRequestDetailPanel({ const [mergeMethod, setMergeMethod] = useState("merge"); const [confirmation, setConfirmation] = useState<{ readonly open: boolean; - readonly action: "merge" | "close" | "enable-auto-merge"; + readonly action: "merge" | "merge-bypass" | "close" | "enable-auto-merge"; }>({ open: false, action: "merge" }); const confirmAction = confirmation.action; // Which handoff is preparing, keyed so a per-finding button can say "Preparing..." on itself @@ -625,6 +630,7 @@ export function PullRequestDetailPanel({ action: PullRequestAction, method?: PullRequestMergeMethod, updateMethod?: PullRequestUpdateMethod, + options?: { readonly bypassRules?: boolean }, ) => { if (pendingAction !== null) return; setPendingAction(action); @@ -635,6 +641,7 @@ export function PullRequestDetailPanel({ action, ...(method ? { mergeMethod: method } : {}), ...(updateMethod ? { updateMethod } : {}), + ...(options?.bypassRules ? { bypassRules: true } : {}), }, }); setPendingAction(null); @@ -646,8 +653,9 @@ export function PullRequestDetailPanel({ const failure = squashAtomCommandFailure(result); // The hint stands for what was actually asked for: a reader who pressed Update branch is // told to check their access, not offered the merge commit they already chose. - const hint = - updateMethod === "rebase" + const hint = options?.bypassRules + ? MERGE_BYPASS_FAILURE_HINT + : updateMethod === "rebase" ? UPDATE_BRANCH_REBASE_FAILURE_HINT : ACTION_FAILURE_HINTS[action]; toastManager.add({ @@ -1084,6 +1092,17 @@ export function PullRequestDetailPanel({ !detail.isDraft && !conflicting && allowedMergeMethods.length > 1; + // The override, offered only where both the host and this account have said it is available. + // Not for a draft or a conflicting branch: neither is a rule that can be stood down, so the + // host refuses those with or without it. + const showsMergeBypass = + detail?.state === "open" && + detail.capabilities.mergeBypass === true && + detail.viewerPermissions.mergeBypass === true && + can("merge") && + !detail.isDraft && + !conflicting && + allowedMergeMethods.length > 0; // The pull request number carries this state in the overview and the right-panel tab mirrors // it. The conflict action is separate from this state: an open pull request remains green. const statePresentation = detail @@ -1435,10 +1454,29 @@ export function PullRequestDetailPanel({ ) : null} + {/* Below the strategies, because it merges with whichever of them is + chosen: it is the same merge with the branch's rules stood down, and + reads as a last resort where it sits last. */} + {showsMergeBypass ? ( + <> + {showsDraftToggle || showsAutoMerge || showsMergeMethods ? ( + + ) : null} + setConfirmation({ open: true, action: "merge-bypass" })} + > + + {`${selectedMergeMethodLabel} past branch rules`} + + + ) : null} {pullRequestActionMenuHasGroup( showsDraftToggle, showsAutoMerge, showsMergeMethods, + showsMergeBypass, ) ? ( ) : null} @@ -1981,19 +2019,26 @@ export function PullRequestDetailPanel({ {confirmAction === "merge" ? "Merge pull request?" - : confirmAction === "enable-auto-merge" - ? "Enable auto-merge?" - : "Close pull request?"} + : confirmAction === "merge-bypass" + ? "Merge past the branch's rules?" + : confirmAction === "enable-auto-merge" + ? "Enable auto-merge?" + : "Close pull request?"} {confirmAction === "merge" ? `This merges #${reference.number} using ${selectedMergeMethod}.` - : confirmAction === "enable-auto-merge" - ? // The host merges this as soon as it considers the pull request ready, which - // may be immediately — there is no telling from here whether anything is - // still outstanding. - `This merges #${reference.number} using ${selectedMergeMethod} as soon as the host considers it ready, which may be immediately.` - : `This closes #${reference.number} without merging it.`} + : // Said as what stops being enforced rather than as what is switched on: the + // reader knows they are an administrator and does not know which of the + // repository's requirements this one is standing on. + confirmAction === "merge-bypass" + ? `This merges #${reference.number} using ${selectedMergeMethod} without the reviews and checks the base branch requires.` + : confirmAction === "enable-auto-merge" + ? // The host merges this as soon as it considers the pull request ready, which + // may be immediately — there is no telling from here whether anything is + // still outstanding. + `This merges #${reference.number} using ${selectedMergeMethod} as soon as the host considers it ready, which may be immediately.` + : `This closes #${reference.number} without merging it.`} @@ -2002,12 +2047,18 @@ export function PullRequestDetailPanel({ diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index a616a8395239..026a61fc28d1 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -91,13 +91,13 @@ export function pullRequestComposerTarget( return context === "thread" ? (target ?? null) : null; } -/** Whether the open pull-request action group contains at least one action. */ -export function pullRequestActionMenuHasGroup( - showsDraftToggle: boolean, - showsAutoMerge: boolean, - showsMergeMethods: boolean, -): boolean { - return showsDraftToggle || showsAutoMerge || showsMergeMethods; +/** + * Whether the open pull-request action group contains at least one action, which is what the + * separator below it is drawn from. Taken as however many the menu has rather than one parameter + * each: the separator's question is only ever "any of them". + */ +export function pullRequestActionMenuHasGroup(...shown: ReadonlyArray): boolean { + return shown.some((entry) => entry); } export function isStackedPullRequestBase( diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 86a8927d4461..cd3774ef836b 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -365,6 +365,12 @@ export const PullRequestCapabilities = Schema.Struct({ actions: Schema.Array(PullRequestAction), /** Merge strategies the provider itself offers, before repository settings narrow them. */ mergeMethods: Schema.Array(PullRequestMergeMethod), + /** + * The host can merge past its own branch rules for someone allowed to override them. Optional + * for the same reason as `updateMethods`: a server that says nothing about it cannot, which is + * what every server before this field was. + */ + mergeBypass: 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. @@ -431,6 +437,12 @@ export const PullRequestViewerPermissions = Schema.Struct({ * Absent or empty means they may not, which is also what a host with no such action says. */ updateMethods: Schema.optional(Schema.Array(PullRequestUpdateMethod)), + /** + * This viewer may merge past the branch's rules. The exception to "a permission the host + * reports nothing about is granted": overriding the rules a repository set for itself is not + * something to offer on the chance that it works, and a host that has not said yes has said no. + */ + mergeBypass: Schema.optional(Schema.Boolean), }); export type PullRequestViewerPermissions = typeof PullRequestViewerPermissions.Type; @@ -874,6 +886,12 @@ export const PullRequestActionInput = Schema.Struct({ mergeMethod: Schema.optional(PullRequestMergeMethod), /** Only read for `update-branch`, where absent means the host's own default. */ updateMethod: Schema.optional(PullRequestUpdateMethod), + /** + * Merge over the branch's own rules, for a viewer the host allows to. Only read for `merge`: + * the deferred merge waits for those rules to be satisfied, so arming one that overrides them + * is a contradiction rather than a shortcut, and every other action answers to different rules. + */ + bypassRules: Schema.optional(Schema.Boolean), }); export type PullRequestActionInput = typeof PullRequestActionInput.Type; diff --git a/packages/contracts/src/vcs.ts b/packages/contracts/src/vcs.ts index a0956e83bd5b..c4829e6edc0d 100644 --- a/packages/contracts/src/vcs.ts +++ b/packages/contracts/src/vcs.ts @@ -77,6 +77,12 @@ export const VcsProcessExitFailureKind = Schema.Literals([ "authentication", "not-found", "rate-limited", + /** The host's own rules on the target branch stand in the way, not the branch's contents. */ + "merge-blocked", + /** The contents do: the two branches collide and no merge commit can be made from them. */ + "merge-conflict", + /** Asked of a change request the host has already merged, which is nobody's mistake to fix. */ + "already-merged", "command-failed", ]); export type VcsProcessExitFailureKind = typeof VcsProcessExitFailureKind.Type; @@ -109,6 +115,36 @@ export class VcsProcessSpawnError extends Schema.TaggedErrorClass()( "VcsProcessExitError", { @@ -132,23 +168,10 @@ export class VcsProcessExitError extends Schema.TaggedErrorClass Date: Sat, 22 Aug 2026 15:45:36 -0400 Subject: [PATCH 2/5] docs(fork): record the merge refusal divergence Signed-off-by: Yordis Prieto --- docs/fork/0024-a-refused-merge-says-why.md | 49 ++++++++++++++++++++++ docs/fork/README.md | 2 + 2 files changed, 51 insertions(+) create mode 100644 docs/fork/0024-a-refused-merge-says-why.md diff --git a/docs/fork/0024-a-refused-merge-says-why.md b/docs/fork/0024-a-refused-merge-says-why.md new file mode 100644 index 000000000000..847710742c92 --- /dev/null +++ b/docs/fork/0024-a-refused-merge-says-why.md @@ -0,0 +1,49 @@ +# 0024: A refused merge says why, and an administrator can merge anyway + +- PR: pending +- Status: active + +## What you can do now + +- Read the reason a merge was refused. A pull request the base branch's rules + hold back, a branch that conflicts, and a pull request somebody already + merged now each say so in the toast, instead of all three arriving as the + same "check that you have write access" guess. +- Merge past the branch's own rules where GitHub lets you. Administrators of a + repository get a "Squash past branch rules" entry in the pull request's + action menu, behind its own confirmation, which does what + `gh pr merge --admin` does. +- Keep the offer out of everyone else's way. The entry appears only where the + host supports the override and the signed-in account administers the + repository, and never on a draft or a conflicting branch, since neither of + those is a rule that can be stood down. + +## Why + +A pull request waiting on a review it will never get is a normal end to a day's +work, and the app answered it with a hint that pointed at the wrong thing. +Write access was fine, the checks were fine, and nothing conflicted: the base +branch simply required an approval. The reason was sitting in the host's own +reply and was being thrown away one layer below the toast, which left the +reader pressing the same button again. + +The override is the other half of the same moment. Somebody who administers the +repository, working alone on their own project, has the authority to merge and +had to leave the app for a terminal to use it. Confirming it separately and +wording it as what stops being enforced keeps that from being a button anyone +presses by accident. + +## Upstream considerations + +Both halves are good upstream candidates and are worth submitting together, as +the second is hard to justify without the first. + +The reason vocabulary follows the shape upstream already uses: no tool output +crosses the process boundary, and each recognized reason gets a sentence +written in the contract. The bypass is optional in the contract and reported +per provider, so GitLab, Bitbucket and Azure DevOps carry on saying nothing and +offering nothing. + +The rebase burden is small but spread across the pull request stack, from the +process boundary to the detail panel. If upstream takes it, the divergence goes +and this entry with it. diff --git a/docs/fork/README.md b/docs/fork/README.md index 3a8968263182..1aa313530455 100644 --- a/docs/fork/README.md +++ b/docs/fork/README.md @@ -63,3 +63,5 @@ Each entry uses these sections: active, [#35](https://github.com/TrogonStack/t3code/pull/35) - **0023** [A service name is not an environment variable](./0023-a-service-name-is-not-an-environment-variable.md) active, [#36](https://github.com/TrogonStack/t3code/pull/36) +- **0024** [A refused merge says why, and an administrator can merge anyway](./0024-a-refused-merge-says-why.md) + active, PR pending From eeddfbdde178c115f320a078be7814a389b414b6 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 22 Aug 2026 15:46:02 -0400 Subject: [PATCH 3/5] docs(fork): link the merge refusal entry to its pull request Signed-off-by: Yordis Prieto --- docs/fork/0024-a-refused-merge-says-why.md | 2 +- docs/fork/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/fork/0024-a-refused-merge-says-why.md b/docs/fork/0024-a-refused-merge-says-why.md index 847710742c92..f0a0fa7c701a 100644 --- a/docs/fork/0024-a-refused-merge-says-why.md +++ b/docs/fork/0024-a-refused-merge-says-why.md @@ -1,6 +1,6 @@ # 0024: A refused merge says why, and an administrator can merge anyway -- PR: pending +- PR: [TrogonStack/t3code#38](https://github.com/TrogonStack/t3code/pull/38) - Status: active ## What you can do now diff --git a/docs/fork/README.md b/docs/fork/README.md index 1aa313530455..74771f970692 100644 --- a/docs/fork/README.md +++ b/docs/fork/README.md @@ -64,4 +64,4 @@ Each entry uses these sections: - **0023** [A service name is not an environment variable](./0023-a-service-name-is-not-an-environment-variable.md) active, [#36](https://github.com/TrogonStack/t3code/pull/36) - **0024** [A refused merge says why, and an administrator can merge anyway](./0024-a-refused-merge-says-why.md) - active, PR pending + active, [#38](https://github.com/TrogonStack/t3code/pull/38) From e3af768bf4d1a17fe7d7e304afd9935118a8c615 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 22 Aug 2026 15:59:04 -0400 Subject: [PATCH 4/5] feat(pull-requests): offer the override where the refusal happened A sentence explaining that the branch's rules forbid the merge is a dead end: the reader is told no and left to find the way out on their own, in a menu they have no reason to open. The refusal now travels as a kind rather than only as prose, so the page can tell the one refusal an administrator can override from the ones nobody can, and ask. Signed-off-by: Yordis Prieto --- .../GitHubPullRequestProvider.test.ts | 32 ++++++++- .../pullRequest/GitHubPullRequestProvider.ts | 5 ++ .../src/pullRequest/PullRequestProvider.ts | 8 ++- .../pullRequest/PullRequestService.test.ts | 40 ++++++++++++ .../src/pullRequest/PullRequestService.ts | 7 +- .../src/sourceControl/GitHubCli.test.ts | 4 ++ apps/server/src/sourceControl/GitHubCli.ts | 10 ++- .../pullRequest/PullRequestDetailPanel.tsx | 29 ++++++++- .../pullRequestDetail.logic.test.ts | 65 +++++++++++++++++++ .../pullRequest/pullRequestDetail.logic.ts | 34 ++++++++++ packages/contracts/src/pullRequest.ts | 15 +++++ 11 files changed, 241 insertions(+), 8 deletions(-) diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index 8bb058d62de9..8aeef9a66720 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -3,10 +3,40 @@ import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; import type { PullRequestReaction } from "@t3tools/contracts"; +import * as GitHubCli from "../sourceControl/GitHubCli.ts"; import * as GitHubPullRequestCli from "./GitHubPullRequestCli.ts"; -import { gitHubViewerPermissions, loginAvatarUrl, make } from "./GitHubPullRequestProvider.ts"; +import { + gitHubProviderFailure, + gitHubViewerPermissions, + loginAvatarUrl, + make, +} from "./GitHubPullRequestProvider.ts"; import type { GitHubReviewThreadComments } from "./gitHubPullRequestJson.ts"; +describe("gitHubProviderFailure", () => { + it("keeps which refusal it was, not only that the request failed", () => { + expect( + gitHubProviderFailure( + new GitHubCli.GitHubCliRefusedError({ + command: "gh", + cwd: "/repo", + cause: null, + refusal: "merge-conflict", + detail: "The two branches conflict, so no merge commit can be created from them.", + }), + ), + ).toEqual({ reason: "failed", refusal: "merge-conflict" }); + }); + + it("says no more than failed about a plain command failure", () => { + expect( + gitHubProviderFailure( + new GitHubCli.GitHubCliCommandError({ command: "gh", cwd: "/repo", cause: null }), + ), + ).toEqual({ reason: "failed" }); + }); +}); + describe("gitHubViewerPermissions", () => { it("offers everything to a viewer who administers the repository", () => { expect( diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index 718bb7bc2537..d2cfdc5e75e5 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -98,6 +98,11 @@ export function gitHubProviderFailure( if (error._tag === "SourceControlRateLimitPausedError") { return { reason: "rate-limited", retryAt: error.retryAt }; } + // A refusal is still a failed request; what it adds is which one, so the page can offer the + // way out where there is one rather than leave the reader with a sentence and no button. + if (error._tag === "GitHubCliRefusedError") { + return { reason: "failed", refusal: error.refusal }; + } return { reason: "failed" }; } diff --git a/apps/server/src/pullRequest/PullRequestProvider.ts b/apps/server/src/pullRequest/PullRequestProvider.ts index 15e07fd7e788..cf340b365d6e 100644 --- a/apps/server/src/pullRequest/PullRequestProvider.ts +++ b/apps/server/src/pullRequest/PullRequestProvider.ts @@ -32,7 +32,10 @@ import type { PullRequestViewerPermissions, SourceControlProviderKind, } from "@t3tools/contracts"; -import { SourceControlProviderKind as SourceControlProviderKindSchema } from "@t3tools/contracts"; +import { + PullRequestRefusal, + SourceControlProviderKind as SourceControlProviderKindSchema, +} from "@t3tools/contracts"; /** * The one failure shape every provider reports, so the service can decide what a failure means @@ -49,6 +52,8 @@ export class PullRequestProviderError extends Schema.TaggedErrorClass }), ); +it.effect("carries the host's refusal out to the client, not just its sentence", () => + Effect.gen(function* () { + const service = yield* makeService({ + projects: [ + project({ id: "p1", title: "t3code", workspaceRoot: "/a", repository: "pingdotgg/t3code" }), + ], + providers: [ + fakeProvider("github", { + runAction: () => + Effect.fail( + new PullRequestProviderError({ + provider: "github", + operation: "runAction", + reason: "failed", + refusal: "merge-blocked", + detail: "The target branch's rules do not allow this merge yet.", + }), + ), + }), + ], + }); + + const error = yield* Effect.flip( + service.runAction({ + projectId: "p1" as ProjectId, + repository: "pingdotgg/t3code", + number: 1, + action: "merge", + }), + ); + + // The sentence is for the reader and the kind is for the page: without it a client offering + // the override would have to recognise the refusal by its wording. + assert.strictEqual(error._tag, "PullRequestOperationError"); + const refused = error._tag === "PullRequestOperationError" ? error : null; + assert.strictEqual(refused?.detail, "The target branch's rules do not allow this merge yet."); + assert.strictEqual(refused?.refusal, "merge-blocked"); + }), +); + it.effect("only lets a merge past the branch's rules when host and account both allow it", () => Effect.gen(function* () { const bypasses: Array = []; diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index f7f105efd809..c3e3e9938891 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -392,7 +392,12 @@ function toPullRequestError( return (error) => isProviderUnusable(error) ? toUnavailableError(error) - : new PullRequestOperationError({ operation, detail: error.detail, cause: error }); + : new PullRequestOperationError({ + operation, + detail: error.detail, + ...(error.refusal === undefined ? {} : { refusal: error.refusal }), + cause: error, + }); } function withRateLimitBackoff( diff --git a/apps/server/src/sourceControl/GitHubCli.test.ts b/apps/server/src/sourceControl/GitHubCli.test.ts index 9302d4eb4bbd..f7a98b5ae11a 100644 --- a/apps/server/src/sourceControl/GitHubCli.test.ts +++ b/apps/server/src/sourceControl/GitHubCli.test.ts @@ -69,6 +69,10 @@ describe("GitHubCli.layer", () => { assert.strictEqual(error._tag, "GitHubCliRefusedError"); assert.strictEqual(error.detail, refusal.detail); + assert.strictEqual( + error._tag === "GitHubCliRefusedError" ? error.refusal : null, + "merge-blocked", + ); assert.strictEqual(error.cause, refusal); }); diff --git a/apps/server/src/sourceControl/GitHubCli.ts b/apps/server/src/sourceControl/GitHubCli.ts index 5b5ccbebae54..4f7d689eabff 100644 --- a/apps/server/src/sourceControl/GitHubCli.ts +++ b/apps/server/src/sourceControl/GitHubCli.ts @@ -6,6 +6,7 @@ import * as Result from "effect/Result"; import * as Schema from "effect/Schema"; import { + PullRequestRefusal, TrimmedNonEmptyString, type SourceControlRepositoryVisibility, type VcsError, @@ -86,7 +87,7 @@ export class GitHubPullRequestNotFoundError extends Schema.TaggedErrorClass()( "GitHubCliRefusedError", - { ...gitHubCliFailureFields, detail: Schema.String }, + { ...gitHubCliFailureFields, refusal: PullRequestRefusal, detail: Schema.String }, ) { override get message(): string { return `GitHub CLI failed in execute: ${this.detail}`; @@ -215,7 +216,12 @@ export function fromVcsError( error.failureKind === "merge-conflict" || error.failureKind === "already-merged" ) { - return new GitHubCliRefusedError({ ...context, detail: error.detail, cause: error }); + return new GitHubCliRefusedError({ + ...context, + refusal: error.failureKind, + detail: error.detail, + cause: error, + }); } } diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 81bd5418c544..1b0880f2b25c 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -111,6 +111,7 @@ import { latestPullRequestReviewOutcomes, isStackedPullRequestBase, pullRequestActionMenuHasGroup, + pullRequestOffersBypassRetry, pullRequestActionNeedsHostRefresh, pullRequestComposerTarget, pullRequestFindingKey, @@ -462,8 +463,11 @@ export function PullRequestDetailPanel({ const [confirmation, setConfirmation] = useState<{ readonly open: boolean; readonly action: "merge" | "merge-bypass" | "close" | "enable-auto-merge"; + /** Set only when the dialog is answering a refusal, and carries what the host said. */ + readonly refusalDetail?: string; }>({ open: false, action: "merge" }); const confirmAction = confirmation.action; + const confirmRefusalDetail = confirmation.refusalDetail; // Which handoff is preparing, keyed so a per-finding button can say "Preparing..." on itself // alone. One at a time whatever the key: they all check the same pull request out. const [handoff, setHandoff] = useState(null); @@ -658,10 +662,25 @@ export function PullRequestDetailPanel({ : updateMethod === "rebase" ? UPDATE_BRANCH_REBASE_FAILURE_HINT : ACTION_FAILURE_HINTS[action]; + const description = readableFailure(failure, hint); + // A merge the branch's rules held back, for a reader who may stand them down, is the one + // failure here with a next step. Offered as the dialog rather than as a toast: the toast + // is gone by the time it has been read, and this is a decision, not a notice. + if ( + pullRequestOffersBypassRetry({ + failure, + action, + attemptedBypass: options?.bypassRules === true, + viewerCanBypass: showsMergeBypass, + }) + ) { + setConfirmation({ open: true, action: "merge-bypass", refusalDetail: description }); + return; + } toastManager.add({ type: "error", title: ACTION_FAILURE_LABELS[action], - description: readableFailure(failure, hint), + description, }); return; } @@ -2020,7 +2039,9 @@ export function PullRequestDetailPanel({ {confirmAction === "merge" ? "Merge pull request?" : confirmAction === "merge-bypass" - ? "Merge past the branch's rules?" + ? confirmRefusalDetail === undefined + ? "Merge past the branch's rules?" + : "Merge anyway?" : confirmAction === "enable-auto-merge" ? "Enable auto-merge?" : "Close pull request?"} @@ -2032,7 +2053,9 @@ export function PullRequestDetailPanel({ // reader knows they are an administrator and does not know which of the // repository's requirements this one is standing on. confirmAction === "merge-bypass" - ? `This merges #${reference.number} using ${selectedMergeMethod} without the reviews and checks the base branch requires.` + ? // After a refusal the host's own sentence leads, because it is the reason the + // dialog is open at all, and the offer reads as an answer to it. + `${confirmRefusalDetail === undefined ? "" : `${confirmRefusalDetail} `}You can merge #${reference.number} using ${selectedMergeMethod} without the reviews and checks the base branch requires.` : confirmAction === "enable-auto-merge" ? // The host merges this as soon as it considers the pull request ready, which // may be immediately — there is no telling from here whether anything is diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index 9b9ef610752b..5864e486de68 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -24,6 +24,8 @@ import { mergePullRequestThreadComments, orderPullRequestComments, pullRequestActionMenuHasGroup, + pullRequestOffersBypassRetry, + pullRequestRefusalOf, pullRequestActionNeedsHostRefresh, pullRequestComposerTarget, pullRequestFindingKey, @@ -128,6 +130,69 @@ describe("pull request action menu", () => { }); }); +describe("pullRequestOffersBypassRetry", () => { + const refused = (refusal: string) => ({ refusal, message: "Pull request operation failed" }); + + it("offers the override to a reader who may use it, on the one refusal it answers", () => { + expect( + pullRequestOffersBypassRetry({ + failure: refused("merge-blocked"), + action: "merge", + attemptedBypass: false, + viewerCanBypass: true, + }), + ).toBe(true); + }); + + it("keeps quiet where the override would change nothing", () => { + // A conflict is the branch's contents and an already merged pull request has nowhere left to + // go: offering a retry for either is offering a second failure. + for (const refusal of ["merge-conflict", "already-merged"]) { + expect( + pullRequestOffersBypassRetry({ + failure: refused(refusal), + action: "merge", + attemptedBypass: false, + viewerCanBypass: true, + }), + ).toBe(false); + } + + // An override that was itself refused, a reader who may not override, and a failure the host + // gave no reason for. + expect( + pullRequestOffersBypassRetry({ + failure: refused("merge-blocked"), + action: "merge", + attemptedBypass: true, + viewerCanBypass: true, + }), + ).toBe(false); + expect( + pullRequestOffersBypassRetry({ + failure: refused("merge-blocked"), + action: "merge", + attemptedBypass: false, + viewerCanBypass: false, + }), + ).toBe(false); + expect( + pullRequestOffersBypassRetry({ + failure: new Error("Pull request operation runAction failed: Something went wrong."), + action: "merge", + attemptedBypass: false, + viewerCanBypass: true, + }), + ).toBe(false); + }); + + it("reads nothing out of a failure that is not one", () => { + expect(pullRequestRefusalOf(null)).toBe(null); + expect(pullRequestRefusalOf("merge-blocked")).toBe(null); + expect(pullRequestRefusalOf({ refusal: "made-up" })).toBe(null); + }); +}); + describe("pull request state description", () => { it("keeps draft and conflicts orthogonal to the terminal states", () => { expect(describePullRequestState("open", true)).toBe("Draft"); diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index 026a61fc28d1..015e6938a201 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -8,6 +8,7 @@ import type { PullRequestDetailView, PullRequestMergeability, PullRequestReaction, + PullRequestRefusal, PullRequestReviewThread, PullRequestState, PullRequestUpdateMethod, @@ -857,6 +858,39 @@ const TOOL_NOISE = [ /^unknown error\.?$/iu, ]; +/** + * The refusal a failed action carried, where it carried one. Read off the decoded error rather + * than out of its sentence: the wording is meant to keep improving, and a client that matches on + * prose breaks every time it does. + */ +export function pullRequestRefusalOf(failure: unknown): PullRequestRefusal | null { + if (typeof failure !== "object" || failure === null) return null; + const refusal = (failure as { readonly refusal?: unknown }).refusal; + return refusal === "merge-blocked" || refusal === "merge-conflict" || refusal === "already-merged" + ? refusal + : null; +} + +/** + * Whether a refused merge is one to offer the override for. Only the rules can be stood down: a + * conflict is the branch's contents and an override does nothing about it, and a pull request + * already merged has nowhere left to go. An override that was itself refused is not offered + * again, because the next press would fail the same way. + */ +export function pullRequestOffersBypassRetry(options: { + readonly failure: unknown; + readonly action: PullRequestAction; + readonly attemptedBypass: boolean; + readonly viewerCanBypass: boolean; +}): boolean { + return ( + options.action === "merge" && + !options.attemptedBypass && + options.viewerCanBypass && + pullRequestRefusalOf(options.failure) === "merge-blocked" + ); +} + /** How much of a host's own message a toast can carry before it stops being read. */ const FAILURE_DETAIL_MAX_LENGTH = 320; diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index cd3774ef836b..8f280b9c0700 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -1181,11 +1181,26 @@ export class PullRequestUnavailableError extends Schema.TaggedErrorClass()( "PullRequestOperationError", { operation: Schema.String, detail: TrimmedNonEmptyString, + /** Absent where the host gave no reason worth acting on, which is most failures. */ + refusal: Schema.optional(PullRequestRefusal), cause: Schema.optional(Schema.Defect()), }, { httpApiStatus: 502 }, From 831ea8babbf4782bca54b5d675349932344a2681 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 22 Aug 2026 15:59:29 -0400 Subject: [PATCH 5/5] docs(fork): record the refusal retry offer Signed-off-by: Yordis Prieto --- docs/fork/0024-a-refused-merge-says-why.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/fork/0024-a-refused-merge-says-why.md b/docs/fork/0024-a-refused-merge-says-why.md index f0a0fa7c701a..5fe77f1b1f57 100644 --- a/docs/fork/0024-a-refused-merge-says-why.md +++ b/docs/fork/0024-a-refused-merge-says-why.md @@ -13,6 +13,11 @@ repository get a "Squash past branch rules" entry in the pull request's action menu, behind its own confirmation, which does what `gh pr merge --admin` does. +- Take the override from where the refusal happened. When a merge is held back + by the base branch's rules and the signed-in account may stand them down, the + toast is replaced by a confirmation that leads with the host's own sentence + and offers the same merge again past those rules. A refusal nobody can + override, and a bypass that was itself refused, still arrive as a toast. - Keep the offer out of everyone else's way. The entry appears only where the host supports the override and the signed-in account administers the repository, and never on a draft or a conflicting branch, since neither of @@ -33,6 +38,12 @@ had to leave the app for a terminal to use it. Confirming it separately and wording it as what stops being enforced keeps that from being a button anyone presses by accident. +Explaining the refusal without offering the way out is still a dead end. The +reader is told no, and the one thing that would get them past it lives in a +menu they have no reason to open at that point. So the reason travels as a kind +and not only as prose, which is what lets the page tell the refusal an +administrator can stand down from the ones nobody can, and only then ask. + ## Upstream considerations Both halves are good upstream candidates and are worth submitting together, as