Skip to content
Merged
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
93 changes: 81 additions & 12 deletions apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,6 @@ describe("GitHubPullRequestProvider.runAction", () => {
input: { action: "update-branch", updateMethod: "rebase" },
args: ["pr", "update-branch", "12", "--repo", "octocat/example-app", "--rebase"],
},
{
name: "disarms auto-merge",
input: { action: "disable-auto-merge" },
args: ["pr", "merge", "12", "--repo", "octocat/example-app", "--disable-auto"],
},
{
name: "deletes the head branch after a merge when asked",
input: { action: "merge", mergeMethod: "merge", deleteBranch: true },
Expand All @@ -64,11 +59,22 @@ describe("GitHubPullRequestProvider.runAction", () => {
);
}

/** The host answers the readiness read with one status and every write with nothing. */
const hostReports = (status: string) => {
/**
* The host answers the readiness read over GraphQL and every write with
* nothing. `isMergeQueueEnabled` is what the queue case turns on.
*/
const hostReports = (status: string, isMergeQueueEnabled = false) => {
mockExecute.mockImplementation((input) =>
Effect.succeed(
processOutput(input.args[1] === "view" ? JSON.stringify({ mergeStateStatus: status }) : ""),
processOutput(
input.args[0] === "api"
? JSON.stringify({
data: {
repository: { pullRequest: { mergeStateStatus: status, isMergeQueueEnabled } },
},
})
: "",
),
),
);
};
Expand All @@ -90,7 +96,6 @@ describe("GitHubPullRequestProvider.runAction", () => {
});

assert.deepStrictEqual(prCalls(), [
["pr", "view", "12", "--repo", "octocat/example-app", "--json", "mergeStateStatus"],
// The strategy travels with the standing instruction.
["pr", "merge", "12", "--repo", "octocat/example-app", "--auto", "--squash"],
]);
Expand All @@ -112,13 +117,77 @@ describe("GitHubPullRequestProvider.runAction", () => {
.pipe(Effect.flip);

assert.equal(error.detail, "This pull request can merge right now. Use Merge instead.");
// Nothing ran but the read: the merge never reached the host.
// Nothing ran but the GraphQL read: the merge never reached the host.
assert.deepStrictEqual(
prCalls().map((args) => args[1]),
["view"],
calls().map((call) => call.args[0]),
["api"],
);
}).pipe(Effect.provide(layer)),
);

/** The host answers the standing read with whether the pull request is queued. */
const hostQueues = (isInMergeQueue: boolean) => {
mockExecute.mockImplementation((input) =>
Effect.succeed(
processOutput(
input.args[0] === "api" && String(input.stdin).includes("isInMergeQueue")
? JSON.stringify({
data: { repository: { pullRequest: { id: "PR_kwDO123", isInMergeQueue } } },
})
: "",
),
),
);
};

it.effect("disarms auto-merge through gh while the pull request is not queued", () =>
Effect.gen(function* () {
hostQueues(false);
const provider = yield* GitHubPullRequestProvider.make();

yield* provider.runAction({ ...repository, number: 12, action: "disable-auto-merge" });

assert.deepStrictEqual(prCalls(), [
["pr", "merge", "12", "--repo", "octocat/example-app", "--disable-auto"],
]);
}).pipe(Effect.provide(layer)),
);

it.effect("takes a queued pull request out of the merge queue by id instead", () =>
Effect.gen(function* () {
hostQueues(true);
const provider = yield* GitHubPullRequestProvider.make();

yield* provider.runAction({ ...repository, number: 12, action: "disable-auto-merge" });

// gh's own disarm returns without touching a queued pull request, so the
// only write is the dequeue mutation and nothing reaches `gh pr`.
assert.deepStrictEqual(prCalls(), []);
const writes = calls().filter((call) => String(call.stdin).includes("dequeuePullRequest"));
assert.equal(writes.length, 1);
assert.match(String(writes[0]?.stdin), /"id":"PR_kwDO123"/);
}).pipe(Effect.provide(layer)),
);

it.effect("arms a green pull request whose base runs a merge queue", () =>
Effect.gen(function* () {
hostReports("CLEAN", true);
const provider = yield* GitHubPullRequestProvider.make();

yield* provider.runAction({
...repository,
number: 12,
action: "enable-auto-merge",
mergeMethod: "squash",
});

// Under a queue this call adds the pull request to it, which is the
// whole point, so the refusal must not stand in the way.
assert.deepStrictEqual(prCalls(), [
["pr", "merge", "12", "--repo", "octocat/example-app", "--auto", "--squash"],
]);
}).pipe(Effect.provide(layer)),
);
});

describe("GitHubPullRequestProvider.submitReview", () => {
Expand Down
120 changes: 73 additions & 47 deletions apps/server/src/pullRequest/GitHubPullRequestProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,27 +32,32 @@ import {
import {
decodeGitHubPullRequestActivityJson,
decodeGitHubPullRequestDetailJson,
decodeGitHubImmediatelyMergeableJson,
decodeGitHubRepositoryJson,
GITHUB_PULL_REQUEST_ACTIVITY_FIELDS,
GITHUB_PULL_REQUEST_DETAIL_FIELDS,
} from "./gitHubPullRequestDetail.ts";
import {
ADD_REACTION_GRAPHQL_MUTATION,
AUTHORED_PULL_REQUESTS_GRAPHQL_QUERY,
BASE_COMPARISON_GRAPHQL_QUERY,
AUTO_MERGE_READINESS_GRAPHQL_QUERY,
buildGitHubReviewerRequestJson,
buildGitHubReviewSubmissionJson,
decodeGitHubAuthoredPullRequestsJson,
decodeGitHubBaseComparisonJson,
decodeGitHubDetailBaseStateJson,
decodeGitHubImmediatelyMergeableJson,
decodeGitHubMergeQueueStandingJson,
decodeGitHubPullRequestConversationJson,
decodeGitHubPullRequestNodeIdJson,
decodeGitHubReviewerCandidatesJson,
decodeGitHubSubjectScopeJson,
DEQUEUE_PULL_REQUEST_GRAPHQL_MUTATION,
DETAIL_BASE_STATE_GRAPHQL_QUERY,
encodeGraphQlRequestJson,
gitHubAuthoredSearchQuery,
gitHubReactionContent,
type GitHubDetailBaseState,
type GitHubGraphQlVariable,
MERGE_QUEUE_STANDING_GRAPHQL_QUERY,
PULL_REQUEST_CONVERSATION_GRAPHQL_QUERY,
PULL_REQUEST_NODE_ID_GRAPHQL_QUERY,
REACTION_SUBJECT_SCOPE_GRAPHQL_QUERY,
Expand Down Expand Up @@ -260,31 +265,42 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () {
stdin: encodeGraphQlRequestJson({ query: input.query, variables: input.variables }),
});

const graphqlRead = <A>(input: {
readonly operation: string;
readonly cwd: string;
readonly query: string;
readonly variables: Readonly<Record<string, GitHubGraphQlVariable>>;
readonly decode: (raw: string) => Result.Result<A, Cause.Cause<Schema.SchemaError>>;
}) =>
graphql(input).pipe(
Effect.flatMap((output) => {
const decoded = input.decode(output.stdout.trim());
return Result.isSuccess(decoded)
? Effect.succeed(decoded.success)
: Effect.fail(decodeError(input.operation, "GraphQL", decoded.failure));
}),
);

/**
* Fails where GitHub would merge the pull request the moment it is armed. The
* refusal is this app's own sentence, since the host never gets asked.
*
* A base guarded by a merge queue is never refused: there the same call adds
* the pull request to the queue rather than merging it, which is exactly what
* arming means under a queue.
*/
const refuseIfImmediatelyMergeable = (
input: ProviderRepositoryRef & { readonly number: number },
) =>
run({
graphqlRead({
operation: "runAction",
cwd: input.cwd,
args: [
"pr",
"view",
String(input.number),
...repositoryArgs(input),
"--json",
"mergeStateStatus",
],
query: AUTO_MERGE_READINESS_GRAPHQL_QUERY,
variables: graphQlVariables(input),
decode: decodeGitHubImmediatelyMergeableJson,
}).pipe(
Effect.flatMap((output) => {
const decoded = decodeGitHubImmediatelyMergeableJson(output.stdout.trim());
if (!Result.isSuccess(decoded)) {
return Effect.fail(decodeError("runAction", "merge state", decoded.failure));
}
return decoded.success
Effect.flatMap((immediatelyMergeable) =>
immediatelyMergeable
? Effect.fail(
new PullRequestProviderError({
provider: PROVIDER_KIND,
Expand All @@ -293,24 +309,8 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () {
detail: "This pull request can merge right now. Use Merge instead.",
}),
)
: Effect.void;
}),
);

const graphqlRead = <A>(input: {
readonly operation: string;
readonly cwd: string;
readonly query: string;
readonly variables: Readonly<Record<string, GitHubGraphQlVariable>>;
readonly decode: (raw: string) => Result.Result<A, Cause.Cause<Schema.SchemaError>>;
}) =>
graphql(input).pipe(
Effect.flatMap((output) => {
const decoded = input.decode(output.stdout.trim());
return Result.isSuccess(decoded)
? Effect.succeed(decoded.success)
: Effect.fail(decodeError(input.operation, "GraphQL", decoded.failure));
}),
: Effect.void,
),
);

/**
Expand Down Expand Up @@ -571,23 +571,25 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () {
graphqlRead({
operation: "detail",
cwd: input.cwd,
query: BASE_COMPARISON_GRAPHQL_QUERY,
query: DETAIL_BASE_STATE_GRAPHQL_QUERY,
variables: {
...graphQlVariables(input),
headRef:
row.headRepositoryOwnerLogin === null
? row.headBranch
: `${row.headRepositoryOwnerLogin}:${row.headBranch}`,
},
decode: decodeGitHubBaseComparisonJson,
decode: decodeGitHubDetailBaseStateJson,
}).pipe(
// A comparison the host will not make leaves the branch's freshness
// unknown; it is not worth failing a detail the reader can use.
Effect.catch(() => Effect.succeed(null)),
Effect.map((behindBy) => ({
// A read the host will not answer leaves the branch's freshness
// unknown and says nothing about a queue; it is not worth failing a
// detail the reader can use.
Effect.catch(() => Effect.succeed<GitHubDetailBaseState>({ behindBy: null })),
Effect.map((base) => ({
...row,
baseComparison: toBaseComparison(behindBy),
behindBy,
baseComparison: toBaseComparison(base.behindBy),
behindBy: base.behindBy,
...(base.mergeQueue === undefined ? {} : { mergeQueue: base.mergeQueue }),
})),
),
),
Expand Down Expand Up @@ -674,9 +676,33 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () {
// `gh pr merge --auto` merges outright when nothing is pending, which is
// not what someone arming a merge asked for. The readiness is read first
// and a ready pull request is sent back to the Merge button instead.
return input.action === "enable-auto-merge"
? refuseIfImmediatelyMergeable(input).pipe(Effect.flatMap(action))
: action();
if (input.action === "enable-auto-merge") {
return refuseIfImmediatelyMergeable(input).pipe(Effect.flatMap(action));
}
// `gh pr merge --disable-auto` stops short on a pull request the host has
// already taken into its merge queue and leaves it there, so that case
// is taken out of the queue by name instead.
if (input.action === "disable-auto-merge") {
return graphqlRead({
operation: "runAction",
cwd: input.cwd,
query: MERGE_QUEUE_STANDING_GRAPHQL_QUERY,
variables: graphQlVariables(input),
decode: decodeGitHubMergeQueueStandingJson,
}).pipe(
Effect.flatMap((queuedId) =>
queuedId === null
? action()
: graphql({
operation: "runAction",
cwd: input.cwd,
query: DEQUEUE_PULL_REQUEST_GRAPHQL_MUTATION,
variables: { id: queuedId },
}).pipe(Effect.asVoid),
),
);
}
return action();
},

comment: (input) =>
Expand Down
6 changes: 6 additions & 0 deletions apps/server/src/pullRequest/PullRequestProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,12 @@ export interface ProviderChangeRequestDetail extends ProviderChangeRequest {
readonly checks: ReadonlyArray<PullRequestCheck>;
/** Absent where the host does not say whether its rules would take a merge. */
readonly mergeGate?: PullRequestMergeGate;
/**
* The merge queue guarding the base, where the host runs one. Absent where
* the base has no queue, or the host does not say. `position` is null until
* this change request has actually joined the queue.
*/
readonly mergeQueue?: { readonly position: number | null };
readonly baseComparison: PullRequestBaseComparison;
/** Null where the host could not compare the branch with its base. */
readonly behindBy: number | null;
Expand Down
1 change: 1 addition & 0 deletions apps/server/src/pullRequest/PullRequestService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,7 @@ function toDetail(input: {
checks: row.checks,
...(row.checksState === undefined ? {} : { checksState: row.checksState }),
...(row.mergeGate === undefined ? {} : { mergeGate: row.mergeGate }),
...(row.mergeQueue === undefined ? {} : { mergeQueue: row.mergeQueue }),
viewer: {
canWrite: input.repository.canWrite,
canReview: viewerKnown && !viewerIsAuthor,
Expand Down
31 changes: 0 additions & 31 deletions apps/server/src/pullRequest/gitHubPullRequestDetail.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,20 +85,6 @@ const GitHubRepositorySchema = Schema.Struct({
default_branch: Schema.optional(Schema.NullOr(Schema.String)),
});

/**
* `gh pr view --json mergeStateStatus`: the one word GitHub sums a pull
* request's readiness into.
*/
const GitHubMergeStateSchema = Schema.Struct({
mergeStateStatus: Schema.optional(Schema.NullOr(Schema.String)),
});

/**
* The states `gh pr merge --auto` merges outright instead of arming, copied
* from the CLI: nothing is pending, so there is nothing to wait for.
*/
const IMMEDIATELY_MERGEABLE_STATES = new Set(["CLEAN", "HAS_HOOKS", "UNSTABLE"]);

/** A `gh pr view` row: everything a list row carries, plus the detail fields. */
export interface GitHubPullRequestDetailRow extends GitHubPullRequestListRow {
readonly body: string;
Expand Down Expand Up @@ -388,7 +374,6 @@ function normalizeCommits(
const decodeDetailPayload = decodeJsonResult(GitHubPullRequestDetailRowSchema);
const decodeActivityPayload = decodeJsonResult(GitHubPullRequestActivitySchema);
const decodeRepositoryPayload = decodeJsonResult(GitHubRepositorySchema);
const decodeMergeStatePayload = decodeJsonResult(GitHubMergeStateSchema);

/**
* Decodes `gh api repos/<owner>/<name>` into the viewer's access and the merge
Expand Down Expand Up @@ -420,22 +405,6 @@ export function decodeGitHubRepositoryJson(
});
}

/**
* Whether GitHub would merge the pull request this instant, read from
* `gh pr view --json mergeStateStatus`. A status the host did not name is not
* ready: arming then waits, which is the safe way to be wrong.
*/
export function decodeGitHubImmediatelyMergeableJson(
raw: string,
): Result.Result<boolean, Cause.Cause<Schema.SchemaError>> {
const payload = decodeMergeStatePayload(raw);
if (!Result.isSuccess(payload)) {
return Result.fail(payload.failure);
}
const status = nonEmptyText(payload.success.mergeStateStatus);
return Result.succeed(status !== null && IMMEDIATELY_MERGEABLE_STATES.has(status));
}

/** Decodes `gh pr view --json <detail fields>` into the header the panel renders. */
export function decodeGitHubPullRequestDetailJson(
raw: string,
Expand Down
Loading
Loading