Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions apps/server/src/auth/RpcAuthorization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ export const RPC_REQUIRED_SCOPES = {
// write like every other one.
[WS_METHODS.pullRequestsReviewerCandidates]: AuthOrchestrationReadScope,
[WS_METHODS.pullRequestsRequestReviewers]: AuthOrchestrationOperateScope,
[WS_METHODS.pullRequestsLabelCandidates]: AuthOrchestrationReadScope,
[WS_METHODS.pullRequestsSetLabels]: AuthOrchestrationOperateScope,
[WS_METHODS.sourceControlLookupRepository]: AuthOrchestrationReadScope,
[WS_METHODS.sourceControlCloneRepository]: AuthOrchestrationOperateScope,
[WS_METHODS.sourceControlPublishRepository]: AuthOrchestrationOperateScope,
Expand Down
66 changes: 64 additions & 2 deletions apps/server/src/pullRequest/GitHubPullRequestCli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2726,7 +2726,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,
canTriage: false,
canUpdate: true,
didAuthor: true,
});
}),
);

Expand Down Expand Up @@ -2889,7 +2894,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,
canTriage: false,
canUpdate: true,
didAuthor: true,
});
yield* TestClock.setTime(Date.parse("2100-01-01T00:00:00Z"));
}),
);
Expand Down Expand Up @@ -3029,4 +3039,56 @@ layer("GitHubPullRequestCli.layer", (it) => {
]);
}),
);

it.effect("puts labels on by posting to the issue's own collection, all at once", () =>
Effect.gen(function* () {
mockedExecute.mockReturnValue(Effect.succeed(output("[]")));
const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli;

yield* cli.setLabels({
cwd: "/w",
repository: "acme/web",
host: "github.com",
number: 7,
labels: ["bug", "size:XL"],
applied: true,
});

assert.strictEqual(mockedExecute.mock.calls.length, 1);
const call = callAt(0);
expect(call.args).toEqual([
"api",
"--method",
"POST",
"--hostname",
"github.com",
"repos/acme/web/issues/7/labels",
"--input",
"-",
]);
// @effect-diagnostics-next-line preferSchemaOverJson:off
Comment thread
macroscopeapp[bot] marked this conversation as resolved.
Outdated
expect(JSON.parse(call.stdin ?? "")).toEqual({ labels: ["bug", "size:XL"] });
}),
);

it.effect("takes labels off one at a time, naming each in the path encoded", () =>
Effect.gen(function* () {
mockedExecute.mockReturnValue(Effect.succeed(output("[]")));
const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli;

yield* cli.setLabels({
cwd: "/w",
repository: "acme/web",
host: "github.com",
number: 7,
labels: ["good first issue", "area/web"],
applied: false,
});

assert.strictEqual(mockedExecute.mock.calls.length, 2);
expect(callAt(0).args).toContain("repos/acme/web/issues/7/labels/good%20first%20issue");
expect(callAt(0).args).toContain("DELETE");
expect(callAt(1).args).toContain("repos/acme/web/issues/7/labels/area%2Fweb");
}),
);
});
72 changes: 72 additions & 0 deletions apps/server/src/pullRequest/GitHubPullRequestCli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
type PullRequestReviewVerdict,
type PullRequestReviewerCandidateList,
type PullRequestReviewerKind,
type PullRequestLabelCandidateList,
type PullRequestThreadCommentsResult,
type PullRequestUpdateMethod,
} from "@t3tools/contracts";
Expand All @@ -42,6 +43,9 @@ import {
decodeReactionSubjectScopeJson,
decodeRepositoryAccessJson,
decodeReviewerCandidatesJson,
decodeLabelCandidatesJson,
buildLabelRequestJson,
LABEL_CANDIDATES_GRAPHQL_QUERY,
decodeReviewDismissalsJson,
decodeReviewThreadCommentsJson,
decodeReviewThreadsJson,
Expand Down Expand Up @@ -597,6 +601,24 @@ export class GitHubPullRequestCli extends Context.Service<
readonly requested: boolean;
}) => Effect.Effect<void, GitHubPullRequestCliError>;

/** The repository's labels, and which of them this pull request already wears. */
readonly listLabelCandidates: (input: {
readonly cwd: string;
readonly repository: string;
readonly host: string;
readonly number: number;
}) => Effect.Effect<PullRequestLabelCandidateList, GitHubPullRequestCliError>;

readonly setLabels: (input: {
readonly cwd: string;
readonly repository: string;
readonly host: string;
readonly number: number;
readonly labels: ReadonlyArray<string>;
/** False takes each label off; true adds each to whatever is already there. */
readonly applied: boolean;
}) => Effect.Effect<void, GitHubPullRequestCliError>;

readonly runPullRequestAction: (input: {
readonly cwd: string;
readonly repository: string;
Expand Down Expand Up @@ -1977,6 +1999,56 @@ export const make = Effect.gen(function* () {
.pipe(Effect.asVoid);
},

listLabelCandidates: (input) => {
const { owner, name } = parseRepositorySelector(input.repository);
return graphqlRead({
cwd: input.cwd,
host: input.host,
operation: "listLabelCandidates",
allowReserve: true,
variables: [
["-f", `owner=${owner}`],
["-f", `name=${name}`],
["-F", `number=${input.number}`],
],
query: LABEL_CANDIDATES_GRAPHQL_QUERY,
decode: decodeLabelCandidatesJson,
});
},

setLabels: (input) => {
const { owner, name } = parseRepositorySelector(input.repository);
// A pull request is an issue to the labels API. Adding posts a list and leaves what was
// already there; taking off is one delete per label, since the endpoint names one in its
// path. The name goes into the path encoded, because a label may carry a space or a slash.
const issue = `repos/${owner}/${name}/issues/${input.number}/labels`;
if (input.applied) {
return github
.execute({
cwd: input.cwd,
args: ["api", "--method", "POST", "--hostname", input.host, issue, "--input", "-"],
stdin: buildLabelRequestJson(input.labels),
})
.pipe(Effect.asVoid);
}
return Effect.forEach(
input.labels,
(label) =>
github.execute({
cwd: input.cwd,
args: [
"api",
"--method",
"DELETE",
"--hostname",
input.host,
`${issue}/${encodeURIComponent(label)}`,
],
}),
{ concurrency: 1, discard: true },
);
},

runPullRequestAction: (input) => {
if (input.action === "revert") {
return pullRequestNodeId({ ...input, operation: "revertPullRequest" }).pipe(
Expand Down
63 changes: 52 additions & 11 deletions apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,14 @@ it.effect("uses one narrow read for a linked pull request summary", () =>

describe("gitHubViewerPermissions", () => {
it("offers everything to a viewer who can write to the repository", () => {
expect(gitHubViewerPermissions({ canWrite: true, canUpdate: true, didAuthor: false })).toEqual({
expect(
gitHubViewerPermissions({
canWrite: true,
canTriage: true,
canUpdate: true,
didAuthor: false,
}),
).toEqual({
// Arming a merge for later is the merge, so it travels with it.
actions: [
"merge",
Expand All @@ -64,33 +71,60 @@ describe("gitHubViewerPermissions", () => {
resolve: true,
verdicts: ["comment", "approve", "request-changes"],
requestReviewers: true,
labels: true,
});
});

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,
canTriage: false,
canUpdate: false,
didAuthor: false,
}),
).toEqual({
actions: [],
comment: true,
resolve: false,
verdicts: ["comment", "approve", "request-changes"],
// Asking somebody else to review is the one thing read access never stretches to.
requestReviewers: false,
labels: false,
});
});

it("lets a triager label without letting them merge or ask for a review", () => {
const permissions = gitHubViewerPermissions({
canWrite: false,
canTriage: true,
canUpdate: false,
didAuthor: false,
});
expect(permissions.labels).toBe(true);
expect(permissions.requestReviewers).toBe(false);
expect(permissions.actions).toEqual([]);
});

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,
canTriage: false,
canUpdate: true,
didAuthor: true,
}),
).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,
resolve: true,
// GitHub refuses an author's approval of their own change, so the page does not offer one.
verdicts: ["comment"],
requestReviewers: false,
labels: false,
});
});

Expand All @@ -110,6 +144,7 @@ describe("gitHubViewerPermissions", () => {
resolve: false,
verdicts: ["comment", "approve", "request-changes"],
requestReviewers: false,
labels: false,
});
expect(detail.workflowApprovalsRequired).toBeUndefined();
expect(detail.checks).toContainEqual({
Expand Down Expand Up @@ -158,7 +193,12 @@ describe("gitHubViewerPermissions", () => {
mergeCapabilities: { merge: true, squash: true, rebase: true },
}),
getViewerAccess: () =>
Effect.succeed({ canWrite: false, canUpdate: true, didAuthor: false }),
Effect.succeed({
canWrite: false,
canTriage: false,
canUpdate: true,
didAuthor: false,
}),
}),
),
),
Expand Down Expand Up @@ -254,7 +294,7 @@ describe("gitHubViewerPermissions", () => {
mergeCapabilities: { merge: true, squash: true, rebase: true },
}),
getViewerAccess: () =>
Effect.succeed({ canWrite: true, canUpdate: true, didAuthor: false }),
Effect.succeed({ canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }),
}),
),
),
Expand Down Expand Up @@ -318,7 +358,7 @@ it.effect("does not classify same-repository gates as fork workflow approvals",
mergeCapabilities: { merge: true, squash: true, rebase: true },
}),
getViewerAccess: () =>
Effect.succeed({ canWrite: true, canUpdate: true, didAuthor: false }),
Effect.succeed({ canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }),
}),
),
),
Expand Down Expand Up @@ -365,7 +405,7 @@ it.effect("keeps an unsafe workflow approval scope visible as unknown", () =>
mergeCapabilities: { merge: true, squash: true, rebase: true },
}),
getViewerAccess: () =>
Effect.succeed({ canWrite: true, canUpdate: true, didAuthor: false }),
Effect.succeed({ canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }),
}),
),
),
Expand Down Expand Up @@ -404,7 +444,7 @@ it.effect("propagates workflow discovery rate limits", () =>
mergeCapabilities: { merge: true, squash: true, rebase: true },
}),
getViewerAccess: () =>
Effect.succeed({ canWrite: true, canUpdate: true, didAuthor: false }),
Effect.succeed({ canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }),
}),
),
),
Expand All @@ -420,7 +460,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, canTriage: true, canUpdate: true, didAuthor: false }),
});

it.effect("offers update-branch when the comparison grants it", () =>
Expand Down Expand Up @@ -466,7 +507,7 @@ describe("getViewerPermissions", () => {
getViewerAccess: (input) =>
Effect.sync(() => {
viewerAllowReserve = input.allowReserve;
return { canWrite: true, canUpdate: true, didAuthor: false };
return { canWrite: true, canTriage: true, canUpdate: true, didAuthor: false };
}),
}),
),
Expand Down Expand Up @@ -501,7 +542,7 @@ describe("getViewerPermissions", () => {
}),
),
getViewerAccess: () =>
Effect.succeed({ canWrite: true, canUpdate: true, didAuthor: false }),
Effect.succeed({ canWrite: true, canTriage: true, canUpdate: true, didAuthor: false }),
}),
),
),
Expand Down
18 changes: 18 additions & 0 deletions apps/server/src/pullRequest/GitHubPullRequestProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ const CAPABILITIES: PullRequestCapabilities = {
},
reviewers: { request: true, listCandidates: true },
edit: { changeRequest: true, comment: true },
labels: true,
};

/**
Expand Down Expand Up @@ -93,6 +94,8 @@ export function gitHubViewerPermissions(access: GitHubViewerAccess): PullRequest
verdicts: access.didAuthor ? (["comment"] as const) : CAPABILITIES.review.verdicts,
requestReviewers: access.canWrite,
...(access.canUpdateBranch === true ? { updateMethods: CAPABILITIES.updateMethods } : {}),
// Triage is the one role that labels without writing, which is what triage is for.
labels: access.canTriage,
};
}

Expand Down Expand Up @@ -526,6 +529,21 @@ export const make = Effect.gen(function* () {
})
.pipe(Effect.mapError(fail("setReviewerRequest"))),

listLabelCandidates: (input) =>
cli.listLabelCandidates(input).pipe(Effect.mapError(fail("listLabelCandidates"))),

setLabels: (input) =>
cli
.setLabels({
cwd: input.cwd,
repository: input.repository,
host: input.host,
number: input.number,
labels: input.labels,
applied: input.applied,
})
.pipe(Effect.mapError(fail("setLabels"))),

runAction: (input) =>
cli
.runPullRequestAction({
Expand Down
Loading
Loading