Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions apps/server/src/pullRequest/GitHubPullRequestApi.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2424,6 +2424,39 @@ layer("GitHubPullRequestApi.layer", (it) => {
}),
);

it.effect.each([
{
description: "a branch still waiting on a review",
action: "enable-auto-merge",
queued: false,
},
{ description: "a merge queue", action: "merge", queued: true },
] as const)("merges directly as an administrator past $description", ({ action, queued }) =>
Effect.gen(function* () {
route([
"query PullRequestActionState",
actionState({ mergeStateStatus: "BLOCKED", isMergeQueueEnabled: queued }),
]);
const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi;

yield* cli.runPullRequestAction({
cwd: "/w",
repository: "acme/web",
host: "github.com",
number: 7,
action,
mergeMethod: "squash",
bypassRequirements: true,
});

// `gh pr merge --admin` is the plain merge mutation; GitHub decides whether it may.
expect(variablesOf("mergePullRequest(")).toEqual([
{ input: { pullRequestId: "PR_7", mergeMethod: "SQUASH" } },
]);
expect(variablesOf("enablePullRequestAutoMerge(")).toEqual([]);
}),
);

it.effect("takes auto-merge back off without naming a strategy", () =>
Effect.gen(function* () {
route([NODE_ID_QUERY, nodeIdAnswer("PR_7")]);
Expand Down
13 changes: 9 additions & 4 deletions apps/server/src/pullRequest/GitHubPullRequestApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,8 @@ export class GitHubPullRequestApi extends Context.Service<
readonly removeAgentCreditsOnMerge?: boolean;
readonly mergeMethod?: PullRequestMergeMethod;
readonly updateMethod?: PullRequestUpdateMethod;
/** Merge now with administrator privileges, as `gh pr merge --admin`. */
readonly bypassRequirements?: boolean;
}) => Effect.Effect<void, GitHubPullRequestApiError>;

readonly commentOnPullRequest: (input: {
Expand Down Expand Up @@ -2663,11 +2665,14 @@ export const make = Effect.gen(function* () {
// A merge queue takes a pull request through auto-merge rather than a direct merge, and
// `--auto` on a pull request that is mergeable right now simply merges it, as `gh` does.
// GitHub stores the strategy with a standing instruction rather than choosing one at
// merge time, so arming still names it.
// merge time, so arming still names it. An administrator's merge is always the direct
// one, past the queue and whatever the branch is still waiting on, as `gh pr merge
// --admin` is: GitHub decides whether this viewer may, and refuses the mutation if not.
const auto =
state.isMergeQueueEnabled === true ||
(action === "enable-auto-merge" &&
!IMMEDIATELY_MERGEABLE.has(state.mergeStateStatus?.toUpperCase() ?? ""));
input.bypassRequirements !== true &&
(state.isMergeQueueEnabled === true ||
(action === "enable-auto-merge" &&
!IMMEDIATELY_MERGEABLE.has(state.mergeStateStatus?.toUpperCase() ?? "")));
yield* graphql({
host: input.host,
operation: "runPullRequestAction",
Expand Down
12 changes: 12 additions & 0 deletions apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,18 @@ describe("gitHubViewerPermissions", () => {
});
});

it("offers the administrator's merge only where GitHub says this viewer may", () => {
const base = { canTriage: true, canUpdate: true, didAuthor: true };
expect(
gitHubViewerPermissions({ ...base, canWrite: true, canMergeAsAdmin: true }).mergeAsAdmin,
).toBe(true);
// Unknown is no: this is a way around the rules, not a permission to grant on a guess.
expect(gitHubViewerPermissions({ ...base, canWrite: true }).mergeAsAdmin).toBeUndefined();
expect(
gitHubViewerPermissions({ ...base, canWrite: false, canMergeAsAdmin: true }).mergeAsAdmin,
).toBeUndefined();
});

it("lets a triager label without letting them merge or ask for a review", () => {
const permissions = gitHubViewerPermissions({
canWrite: false,
Expand Down
4 changes: 4 additions & 0 deletions apps/server/src/pullRequest/GitHubPullRequestProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ export function gitHubViewerPermissions(access: GitHubViewerAccess): PullRequest
verdicts: access.didAuthor ? (["comment"] as const) : CAPABILITIES.review.verdicts,
requestReviewers: access.canWrite,
...(access.canUpdateBranch === true ? { updateMethods: CAPABILITIES.updateMethods } : {}),
// Only where GitHub says so outright: bypassing a required review is not something to offer
// on a guess, and whoever may not merge at all may not merge past the rules either.
...(access.canWrite && access.canMergeAsAdmin === true ? { mergeAsAdmin: true } : {}),
// Triage is the one role that labels without writing, which is what triage is for.
labels: access.canTriage,
};
Expand Down Expand Up @@ -600,6 +603,7 @@ export const make = Effect.gen(function* () {
: { expectedStackHeads: input.expectedStackHeads }),
...(input.mergeMethod === undefined ? {} : { mergeMethod: input.mergeMethod }),
...(input.updateMethod === undefined ? {} : { updateMethod: input.updateMethod }),
...(input.bypassRequirements === true ? { bypassRequirements: true } : {}),
})
.pipe(Effect.mapError(fail("runAction"))),

Expand Down
5 changes: 5 additions & 0 deletions apps/server/src/pullRequest/PullRequestProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,11 @@ export interface PullRequestProviderApi {
readonly mergeMethod?: PullRequestMergeMethod;
/** Only meaningful for `update-branch`; absent takes the host's own default. */
readonly updateMethod?: PullRequestUpdateMethod;
/**
* Merge now, past the branch's protections, for `merge` and `enable-auto-merge`. Only
* passed where the viewer's permissions grant `mergeAsAdmin`, which only GitHub reports.
*/
readonly bypassRequirements?: boolean;
},
) => Effect.Effect<void, PullRequestProviderError>;

Expand Down
65 changes: 65 additions & 0 deletions apps/server/src/pullRequest/PullRequestService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1562,6 +1562,71 @@ it.effect("gates arming a merge for later exactly as it gates merging now", () =
}),
);

it.effect("passes an administrator's merge on only to a viewer the host says may make one", () =>
Effect.gen(function* () {
let mergeAsAdmin = false;
let ranWith: { readonly action: string; readonly bypassRequirements?: boolean } | null = null;
const service = yield* makeService({
projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })],
providers: [
fakeProvider("github", {
capabilities: {
diff: true,
comment: true,
actions: ["merge", "close", "enable-auto-merge"],
mergeMethods: ["merge", "squash"],
search: true,
reactions: true,
review: FULL_REVIEW,
reviewers: FULL_REVIEWERS,
},
getViewerPermissions: () =>
Effect.succeed({
actions: ["merge", "enable-auto-merge", "close"],
comment: true,
resolve: true,
verdicts: ["comment", "approve", "request-changes"],
requestReviewers: true,
...(mergeAsAdmin ? { mergeAsAdmin: true } : {}),
}),
runAction: (input) => {
ranWith = {
action: input.action,
...(input.bypassRequirements === undefined
? {}
: { bypassRequirements: input.bypassRequirements }),
};
return Effect.void;
},
}),
],
});
const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 };

const refused = yield* Effect.flip(
service.runAction({ ...reference, action: "merge", bypassRequirements: true }),
);
assert.strictEqual(refused._tag, "PullRequestOperationError");
assert.include(refused.message, "merge past this branch's protections");
assert.strictEqual(ranWith, null);

mergeAsAdmin = true;
// Only a merge goes around the rules; nothing else may borrow the flag.
const wrongAction = yield* Effect.flip(
service.runAction({ ...reference, action: "close", bypassRequirements: true }),
);
assert.strictEqual(wrongAction._tag, "PullRequestOperationError");
assert.strictEqual(ranWith, null);

yield* service.runAction({
...reference,
action: "enable-auto-merge",
bypassRequirements: true,
});
assert.deepStrictEqual(ranWith, { action: "enable-auto-merge", bypassRequirements: true });
}),
);

it.effect("hands the host the strategy an armed merge was asked for", () =>
Effect.gen(function* () {
let ranWith: { readonly action: string; readonly mergeMethod?: string } | null = null;
Expand Down
16 changes: 16 additions & 0 deletions apps/server/src/pullRequest/PullRequestService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2046,6 +2046,21 @@ export const make = Effect.gen(function* () {
}),
);
}
// Merging past the rules is asked for by name and granted by name: it is refused for
// anything but a single merge, and for anyone the host did not say may do it.
if (
input.bypassRequirements === true &&
(input.stackNumber !== undefined ||
(input.action !== "merge" && input.action !== "enable-auto-merge") ||
viewer.mergeAsAdmin !== true)
) {
return Effect.fail(
new PullRequestOperationError({
operation: "runAction",
detail: "You do not have permission to merge past this branch's protections.",
}),
);
}
const mergeSettings =
project.api.kind === "github" &&
input.stackNumber === undefined &&
Expand Down Expand Up @@ -2083,6 +2098,7 @@ export const make = Effect.gen(function* () {
...(input.updateMethod === undefined
? {}
: { updateMethod: input.updateMethod }),
...(input.bypassRequirements === true ? { bypassRequirements: true } : {}),
})
.pipe(
// Once the authorized provider action starts, a failure may leave partial
Expand Down
12 changes: 12 additions & 0 deletions apps/server/src/pullRequest/gitHubPullRequestJson.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,18 @@ describe("viewer permission decoding", () => {
});
});

it("reads whether this viewer may merge past the branch's protections", () => {
const decode = (pullRequest: Record<string, unknown> | null) =>
expectSuccess(
decodeViewerPermissionsJson(viewerJson({ viewerPermission: "ADMIN", pullRequest })),
).canMergeAsAdmin;
expect(decode({ viewerCanMergeAsAdmin: true })).toBe(true);
// An install that does not report it, or says no, grants nothing.
expect(decode({ viewerCanMergeAsAdmin: false })).toBeUndefined();
expect(decode({})).toBeUndefined();
expect(decode(null)).toBeUndefined();
});

it("says no to a passer-by on a repository they can only read", () => {
expect(
expectSuccess(
Expand Down
21 changes: 18 additions & 3 deletions apps/server/src/pullRequest/gitHubPullRequestJson.ts
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,8 @@ const RawCoreSchema = Schema.Struct({
...RawDetailSchema.fields,
...RawViewerFieldsSchema.fields,
viewerCanUpdateBranch: Schema.Boolean,
/** Optional so an install that does not report it reads as "may not", not a failed read. */
viewerCanMergeAsAdmin: Schema.optional(Schema.Boolean),
baseRef: Schema.NullOr(
Schema.Struct({
compare: Schema.NullOr(Schema.Struct({ behindBy: Schema.Int })),
Expand Down Expand Up @@ -832,7 +834,7 @@ export const pullRequestCoreGraphQlQuery = (host: string) => {
headRepositoryOwner { login }
author { login avatarUrl ... on User { id name } }
autoMergeRequest { mergeMethod }
viewerCanUpdate viewerDidAuthor viewerCanUpdateBranch
viewerCanUpdate viewerDidAuthor viewerCanUpdateBranch viewerCanMergeAsAdmin
baseRef { compare(headRef: $headRef) { behindBy } }
reviewRequests(first: 100) {
nodes { requestedReviewer { ... on User { login name } ... on Bot { login } ... on Team { slug name } } }
Expand Down Expand Up @@ -2320,6 +2322,7 @@ export function decodePullRequestCoreJson(
canWrite: toCanWrite(repository.viewerPermission),
canTriage: toCanTriage(repository.viewerPermission),
...toPullRequestViewerFields(pr),
...(pr.viewerCanMergeAsAdmin === true ? { canMergeAsAdmin: true } : {}),
mergeCapabilities: {
merge: repository.mergeCommitAllowed,
squash: repository.squashMergeAllowed,
Expand Down Expand Up @@ -3153,13 +3156,19 @@ export interface GitHubViewerAccess {
* something to update" at once. Absent where the comparison was not read.
*/
readonly canUpdateBranch?: boolean;
/**
* GitHub's own `viewerCanMergeAsAdmin`: this viewer may merge past the branch's protections,
* which is what `gh pr merge --admin` does. Absent reads as "may not" — unlike the rest, it is
* a way around the rules rather than through them, so an unknown answer grants nothing.
*/
readonly canMergeAsAdmin?: boolean;
}

/** Core detail and write checks share one read of permissions and merge settings. */
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 viewerCanMergeAsAdmin }
}
}`;

Expand All @@ -3168,7 +3177,12 @@ 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,
viewerCanMergeAsAdmin: Schema.optional(Schema.Boolean),
}),
),
}),
}),
});
Expand All @@ -3192,6 +3206,7 @@ export function decodeViewerPermissionsJson(
canWrite: toCanWrite(repository.viewerPermission),
canTriage: toCanTriage(repository.viewerPermission),
...toPullRequestViewerFields(repository.pullRequest),
...(repository.pullRequest?.viewerCanMergeAsAdmin === true ? { canMergeAsAdmin: true } : {}),
});
}

Expand Down
Loading
Loading