diff --git a/docs/user/source-control.md b/docs/user/source-control.md index bd7715c044a3..651ab18a40ad 100644 --- a/docs/user/source-control.md +++ b/docs/user/source-control.md @@ -116,7 +116,7 @@ kept in the server's secret store, applies right away, and is used before `CAFE_ sent to `git.cafe`; `CAFE_TOKEN` follows `CAFE_HOST` if you set it. Repositories on `staging.git.cafe` need their own login with `--host https://staging.git.cafe/api`. -Line comments, reviewer requests, and labels are not available for GitCafe pull requests yet. +Line comments are not available for GitCafe pull requests yet. Merging a stack lands it through GitCafe, and updating a branch restacks the layers above it. ## Start, clone, or publish a project diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts index d31f73038f20..35d18eec2514 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts @@ -52,9 +52,11 @@ function fakeGitCafe( const response = route === undefined ? new Response(encodeJson({ type: "https://cafe.sh/errors/not-found" }), { status: 404 }) - : new Response(encodeJson(typeof route === "function" ? route(entry) : route), { - status: options.status ?? 200, - }); + : route instanceof Response + ? route + : new Response(encodeJson(typeof route === "function" ? route(entry) : route), { + status: options.status ?? 200, + }); return Effect.succeed(HttpClientResponse.fromWeb(request, response)); }, ); @@ -104,6 +106,10 @@ const pull = { sourceRepo: null, capabilities: { comment: true, review: true, merge: true, edit: true, moderate: true }, }; +const labels = [ + { id: "label_bug", name: "bug", color: "#d73a4a", description: null }, + { id: "label_docs", name: "docs", color: "#0075ca", description: "Documentation" }, +]; const root = { id: "root", threadId: "root", @@ -439,16 +445,18 @@ describe("GitCafePullRequestProvider", () => { }, ); - it.effect("reads line counts, open merge strategies and requested reviewers", () => { + it.effect("reads line counts, open merge strategies, reviewers and labels", () => { const snapshot = { version: 4, headOid, comparisonBaseOid: baseOid }; const files = (from: number, count: number) => Array.from({ length: count }, (_, index) => ({ path: `f${from + index}.ts` })); let changePage = 0; const server = fakeGitCafe({ - "GET /repos/owner/repo/pulls/7": { ...pull, reviewers: [{ actor }] }, + "GET /repos/owner/repo/pulls/7": pull, + "GET /repos/owner/repo/pulls/7/reviewers": { items: [{ id: "act_one", actor }], next: null }, + "GET /repos/owner/repo/pulls/7/labels": { items: [labels[0]], next: null }, "GET /repos/owner/repo/pulls/7/status": { merge: { - conflicts: "unknown", + conflicts: "none", fastForward: true, strategies: ["merge", "squash", "rebase"], blockers: [{ blockedStrategies: ["squash"] }, {}], @@ -483,7 +491,12 @@ describe("GitCafePullRequestProvider", () => { return Effect.gen(function* () { const provider = yield* GitCafePullRequestProvider.make; const detail = yield* provider.getChangeRequest(target); - assert.deepInclude(detail, { additions: 6, deletions: 3, changedFiles: 3 }); + assert.deepInclude(detail, { + additions: 6, + deletions: 3, + changedFiles: 3, + mergeability: "mergeable", + }); assert.strictEqual( server.sent.filter((request) => request.path.endsWith("/changes"))[1]?.query.get("after"), "cursor-1", @@ -501,6 +514,143 @@ describe("GitCafePullRequestProvider", () => { detail.reviewers.map((reviewer) => reviewer.login), ["alice"], ); + assert.deepStrictEqual(detail.reviewRequestLogins, ["alice"]); + assert.deepStrictEqual(detail.labels, [{ name: "bug", color: "#d73a4a" }]); + }).pipe(Effect.provide(server.layer)); + }); + + it.effect("still reads a pull request whose reviewers the token may not see", () => { + const server = fakeGitCafe({ + "GET /repos/owner/repo/pulls/7": { ...pull, headOid: null }, + "GET /repos/owner/repo/pulls/7/reviewers": new Response( + encodeJson({ type: "https://cafe.sh/errors/forbidden" }), + { status: 403 }, + ), + "GET /repos/owner/repo/pulls/7/labels": { items: [labels[0]], next: null }, + "GET /repos/owner/repo/pulls/7/status": { + merge: { conflicts: "unknown", fastForward: null, strategies: [] }, + checks: { pending: 0, failing: 0, total: 0 }, + }, + }); + return Effect.gen(function* () { + const provider = yield* GitCafePullRequestProvider.make; + const detail = yield* provider.getChangeRequest(target); + assert.deepStrictEqual(detail.reviewers, []); + assert.deepStrictEqual(detail.labels, [{ name: "bug", color: "#d73a4a" }]); + }).pipe(Effect.provide(server.layer)); + }); + + it.effect("fails the detail rather than hide reviewers GitCafe could not serve", () => { + const server = fakeGitCafe({ + "GET /repos/owner/repo/pulls/7": { ...pull, headOid: null }, + "GET /repos/owner/repo/pulls/7/reviewers": new Response("", { status: 503 }), + "GET /repos/owner/repo/pulls/7/labels": { items: [], next: null }, + "GET /repos/owner/repo/pulls/7/status": { + merge: { conflicts: "unknown", fastForward: null, strategies: [] }, + checks: { pending: 0, failing: 0, total: 0 }, + }, + }); + return Effect.gen(function* () { + const provider = yield* GitCafePullRequestProvider.make; + const result = yield* provider.getChangeRequest(target).pipe(Effect.result); + assert.strictEqual(result._tag, "Failure"); + }).pipe(Effect.provide(server.layer)); + }); + + it.effect("requests and withdraws reviewers by rewriting the whole list", () => { + const bob = { ...actor, actorId: "act_two", handle: "bob" }; + let requested = [{ id: "act_two", actor: bob }]; + const server = fakeGitCafe({ + "GET /repos/owner/repo/pulls/7": pull, + "GET /repos/owner/repo/pulls/7/reviewer-candidates": { + items: [ + { id: "act_one", actor }, + { id: "act_two", actor: bob }, + { id: "act_bot", actor: { ...actor, actorId: "act_bot", handle: "lint", bot: true } }, + ], + next: null, + }, + "GET /repos/owner/repo/pulls/7/reviewers": () => ({ items: requested, next: null }), + "PUT /repos/owner/repo/pulls/7/reviewers": { version: 5, items: [] }, + }); + return Effect.gen(function* () { + const provider = yield* GitCafePullRequestProvider.make; + const list = yield* provider.listReviewerCandidates(target); + // The author, alice, cannot review her own pull request. + assert.deepStrictEqual( + list.candidates.map((c) => [c.id, c.login, c.isRequested, c.isBot ?? false]), + [ + ["act_two", "bob", true, false], + ["act_bot", "lint", false, true], + ], + ); + + const set = (id: string, on: boolean) => + provider.setReviewerRequest({ + ...target, + reviewers: [{ id, kind: "user" }], + requested: on, + }); + yield* set("act_bot", true); + assert.deepStrictEqual(server.sent.at(-1)?.body, { + expectedVersion: 4, + actorIds: ["act_two", "act_bot"], + }); + yield* set("act_two", false); + assert.deepStrictEqual(server.sent.at(-1)?.body, { expectedVersion: 4, actorIds: [] }); + + requested = []; + yield* set("act_two", false); + assert.strictEqual(server.writes().length, 2); + }).pipe(Effect.provide(server.layer)); + }); + + it.effect("names the missing scopes when the token may not request reviewers", () => { + const server = fakeGitCafe({ + "GET /repos/owner/repo/pulls/7": pull, + "GET /repos/owner/repo/pulls/7/reviewer-candidates": new Response("", { status: 403 }), + "GET /repos/owner/repo/pulls/7/reviewers": { items: [], next: null }, + }); + return Effect.gen(function* () { + const provider = yield* GitCafePullRequestProvider.make; + const error = yield* Effect.flip(provider.listReviewerCandidates(target)); + assert.match(error.detail, /pr:reviewers:write/u); + }).pipe(Effect.provide(server.layer)); + }); + + it.effect("applies and removes labels by name through their ids", () => { + const server = fakeGitCafe({ + "GET /repos/owner/repo/pulls/7": pull, + "GET /repos/owner/repo/labels": { items: labels, next: null }, + "GET /repos/owner/repo/pulls/7/labels": { items: [labels[0]], next: null }, + "PUT /repos/owner/repo/pulls/7/labels": { version: 5, items: [] }, + }); + return Effect.gen(function* () { + const provider = yield* GitCafePullRequestProvider.make; + const list = yield* provider.listLabelCandidates!(target); + assert.deepStrictEqual( + list.candidates.map((label) => [label.name, label.isApplied]), + [ + ["bug", true], + ["docs", false], + ], + ); + + yield* provider.setLabels!({ ...target, labels: ["docs"], applied: true }); + assert.deepStrictEqual(server.sent.at(-1)?.body, { + expectedVersion: 4, + labelIds: ["label_bug", "label_docs"], + }); + yield* provider.setLabels!({ ...target, labels: ["bug"], applied: false }); + assert.deepStrictEqual(server.sent.at(-1)?.body, { expectedVersion: 4, labelIds: [] }); + + const missing = yield* provider.setLabels!({ + ...target, + labels: ["nope"], + applied: true, + }).pipe(Effect.result); + assert.strictEqual(missing._tag, "Failure"); + assert.strictEqual(server.writes().length, 2); }).pipe(Effect.provide(server.layer)); }); diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts index 41683de1767d..02c2af98f8e6 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts @@ -31,7 +31,7 @@ const CAPABILITIES: PullRequestCapabilities = { stacks: true, stackActions: true, reactions: true, - labels: false, + labels: true, // Line comments go through GitCafe's review drafts, which arrive with revision fencing. review: { inlineComment: false, @@ -39,7 +39,7 @@ const CAPABILITIES: PullRequestCapabilities = { resolve: true, verdicts: ["approve", "request-changes", "comment"], }, - reviewers: { request: false, listCandidates: false }, + reviewers: { request: true, listCandidates: true }, edit: { changeRequest: true, comment: true }, }; @@ -71,13 +71,16 @@ const mergeStrategyOpen = (status: typeof Status.Type, strategy: string) => const Id = TrimmedNonEmptyString.check(Schema.isPattern(/^[A-Za-z0-9_-]+$/u)); const isId = Schema.is(Id); +const isApiError = Schema.is(GitCafeApi.GitCafeApiError); const Principal = Schema.Struct({ handle: TrimmedNonEmptyString }); const Written = Schema.Struct({ id: Id }); const Versioned = Schema.Struct({ version: NonNegativeInt }); const CommentDetail = Schema.Struct({ id: Id, version: NonNegativeInt }); const Status = Schema.Struct({ merge: Schema.Struct({ - conflicts: Schema.Literals(["unknown", "conflicting"]), + // The API reference lists "unknown" and "conflicting", but git.cafe also sends "none" and + // "pending", so any state is read and only the known ones are mapped. + conflicts: Schema.String, fastForward: Schema.NullOr(Schema.Boolean), strategies: Schema.Array(Schema.String), permitted: Schema.optional(Schema.Boolean), @@ -93,6 +96,10 @@ const Status = Schema.Struct({ total: NonNegativeInt, }), }); +const Listed = Schema.Struct({ + items: Schema.Array(Schema.Struct({ id: TrimmedNonEmptyString })), + next: Schema.NullOr(Schema.String), +}); const FilterOptions = Schema.Struct({ actors: Schema.Array( Schema.Struct({ actorId: TrimmedNonEmptyString, handle: TrimmedNonEmptyString }), @@ -254,6 +261,62 @@ export const make = Effect.gen(function* () { ), ); + const isForbidden = (error: PullRequestProviderError) => + isApiError(error.cause) && error.cause.status === 403; + + /** + * GitCafe offers no way to read a token's scopes, so reviewer controls show + * for anyone who may edit the pull request; a token without the reviewer + * scopes learns which ones it lacks when it uses them. + */ + const explainReviewerScopes = + (operation: string) => + (effect: Effect.Effect) => + effect.pipe( + Effect.catchIf(isForbidden, (error) => + Effect.fail( + failure( + operation, + "This GitCafe token cannot request reviewers. It needs the pr:review:read and pr:reviewers:write scopes.", + error, + ), + ), + ), + ); + + /** + * GitCafe replaces a pull request's whole reviewer or label list, so one toggle reads the + * list, changes it, and writes it back fenced on the pull request's version. The version is + * read first: a change landing between the two reads then fails the fence instead of being + * overwritten. + */ + const toggleListed = Effect.fn("GitCafePullRequestProvider.toggleListed")(function* ( + operation: string, + input: PullRef, + list: { readonly name: "reviewers" | "labels"; readonly key: "actorIds" | "labelIds" }, + ids: ReadonlyArray, + on: boolean, + ) { + const path = yield* pullPath(operation, input); + const pull = yield* readPull(operation, input); + const listed = yield* readPages(operation, input, `${path}/${list.name}`, Listed); + const current = listed.items.map((item) => item.id); + const next = on + ? [...new Set([...current, ...ids])] + : current.filter((id) => !ids.includes(id)); + if (next.length === current.length) return; + yield* read( + operation, + input, + { + method: "PUT", + path: `${path}/${list.name}`, + body: { expectedVersion: pull.version, [list.key]: next }, + }, + Versioned, + ); + }); + /** * GitCafe's stack lists no heads, and they are what a land or restack is fenced on, so each * open layer's pull is read. @@ -537,7 +600,8 @@ export const make = Effect.gen(function* () { const pull = yield* readPull("getChangeRequest", input); const path = yield* pullPath("getChangeRequest", input); const base = yield* repositoryPath("getChangeRequest", input); - const [status, checks, changes] = yield* Effect.all( + // A single pull request read carries neither its reviewers nor its labels. + const [status, checks, changes, reviewers, labels] = yield* Effect.all( [ read("getChangeRequest", input, { path: `${path}/status` }, Status), pull.headOid === null @@ -551,12 +615,20 @@ export const make = Effect.gen(function* () { pull.headOid === null ? Effect.succeed(null) : readLineStats(input, pull.version).pipe(Effect.orElseSucceed(() => null)), + // Reviewers need `pr:review:read`, which a contents-only token lacks. + readPages("getChangeRequest", input, `${path}/reviewers`, Json.GitCafeActorEntries).pipe( + Effect.catch((error) => + isForbidden(error) ? Effect.succeed({ items: [], next: null }) : Effect.fail(error), + ), + ), + readPages("getChangeRequest", input, `${path}/labels`, Json.GitCafeLabels), ], - { concurrency: 3 }, + { concurrency: 5 }, ); + const listed = { ...pull, reviewers: reviewers.items, labels: labels.items }; const permissions = Json.toViewerPermissions(pull); return { - ...Json.toChangeRequest(pull, input), + ...Json.toChangeRequest(listed, input), headSha: pull.headOid, body: pull.description ?? "", ...(pull.sourceRepo === null @@ -567,7 +639,7 @@ export const make = Effect.gen(function* () { changedFiles: changes?.changedFiles ?? 0, closedAt: pull.closedAt, mergedAt: pull.mergedAt, - reviewers: Json.toReviewers(pull, input.host), + reviewers: Json.toReviewers(listed, input.host), checks: checks === null ? [] : Json.toChecks(checks), ...(status.checks.total === 0 ? {} @@ -582,7 +654,9 @@ export const make = Effect.gen(function* () { mergeability: status.merge.conflicts === "conflicting" ? ("conflicting" as const) - : ("unknown" as const), + : status.merge.conflicts === "none" + ? ("mergeable" as const) + : ("unknown" as const), baseComparison: status.merge.fastForward === null ? ("unknown" as const) @@ -758,12 +832,80 @@ export const make = Effect.gen(function* () { Written, ); }), - listReviewerCandidates: () => - Effect.fail( - failure("listReviewerCandidates", "GitCafe reviewer requests are not supported."), - ), - setReviewerRequest: () => - Effect.fail(failure("setReviewerRequest", "GitCafe reviewer requests are not supported.")), + listReviewerCandidates: Effect.fn("GitCafePullRequestProvider.listReviewerCandidates")( + function* (input) { + const operation = "listReviewerCandidates"; + const path = yield* pullPath(operation, input); + const [pull, candidates, reviewers] = yield* Effect.all( + [ + readPull(operation, input), + readPages( + operation, + input, + `${path}/reviewer-candidates`, + Json.GitCafeActorEntries, + ).pipe(explainReviewerScopes(operation)), + readPages(operation, input, `${path}/reviewers`, Listed).pipe( + explainReviewerScopes(operation), + ), + ], + { concurrency: 3 }, + ); + return { + candidates: Json.toReviewerCandidates(candidates, { + requested: new Set(reviewers.items.map((reviewer) => reviewer.id)), + authorActorId: pull.author.actorId, + host: input.host, + }), + truncated: candidates.next !== null, + }; + }, + ), + setReviewerRequest: (input) => + toggleListed( + "setReviewerRequest", + input, + { name: "reviewers", key: "actorIds" }, + input.reviewers.map((reviewer) => reviewer.id), + input.requested, + ).pipe(explainReviewerScopes("setReviewerRequest")), + listLabelCandidates: Effect.fn("GitCafePullRequestProvider.listLabelCandidates")( + function* (input) { + const operation = "listLabelCandidates"; + const base = yield* repositoryPath(operation, input); + const [labels, applied] = yield* Effect.all( + [ + readPages(operation, input, `${base}/labels`, Json.GitCafeLabels), + readPages(operation, input, `${base}/pulls/${input.number}/labels`, Listed), + ], + { concurrency: 2 }, + ); + const appliedIds = new Set(applied.items.map((label) => label.id)); + return { + candidates: labels.items.map((label) => ({ + name: label.name, + color: label.color, + description: label.description ?? null, + isApplied: appliedIds.has(label.id), + })), + truncated: labels.next !== null, + }; + }, + ), + setLabels: Effect.fn("GitCafePullRequestProvider.setLabels")(function* (input) { + const base = yield* repositoryPath("setLabels", input); + const labels = yield* readPages("setLabels", input, `${base}/labels`, Json.GitCafeLabels); + const selected = labels.items.filter((label) => input.labels.includes(label.name)); + if (selected.length !== input.labels.length) + return yield* failure("setLabels", "One or more requested labels could not be found."); + yield* toggleListed( + "setLabels", + input, + { name: "labels", key: "labelIds" }, + selected.map((label) => label.id), + input.applied, + ); + }), replyToThread: Effect.fn("GitCafePullRequestProvider.replyToThread")(function* (input) { if (!isId(input.threadId)) return yield* failure("replyToThread", "GitCafe thread ids are opaque slugs."); diff --git a/packages/source-control-gitcafe/src/server/gitCafePullRequestJson.ts b/packages/source-control-gitcafe/src/server/gitCafePullRequestJson.ts index 66d1bf3a2b33..2b848639a04c 100644 --- a/packages/source-control-gitcafe/src/server/gitCafePullRequestJson.ts +++ b/packages/source-control-gitcafe/src/server/gitCafePullRequestJson.ts @@ -12,6 +12,7 @@ import { type PullRequestComment, type PullRequestReaction, type PullRequestReactionContent, + type PullRequestReviewerCandidate, type PullRequestReviewThread, type PullRequestViewerPermissions, } from "@t3tools/contracts"; @@ -29,6 +30,7 @@ const Actor = Schema.Union([ handle: TrimmedNonEmptyString, displayName: Schema.NullOr(Schema.String), avatarUrl: Schema.NullOr(Schema.String), + bot: Schema.optional(Schema.Boolean), }), Schema.Struct({ kind: Schema.Literal("github"), @@ -143,6 +145,24 @@ export const GitCafeCommits = Schema.Struct({ headOid: Schema.String, }); +export const GitCafeLabels = Schema.Struct({ + items: Schema.Array( + Schema.Struct({ + id: TrimmedNonEmptyString, + name: TrimmedNonEmptyString, + color: Schema.NullOr(Schema.String), + description: Schema.optional(Schema.NullOr(Schema.String)), + }), + ), + next: Schema.NullOr(Schema.String), +}); + +/** A pull request's reviewers and its reviewer candidates, each actor under its id. */ +export const GitCafeActorEntries = Schema.Struct({ + items: Schema.Array(Schema.Struct({ id: TrimmedNonEmptyString, actor: Actor })), + next: Schema.NullOr(Schema.String), +}); + export const GitCafeReactions = Schema.Struct({ items: Schema.Array( Schema.Struct({ @@ -249,6 +269,7 @@ function toActor(actor: Actor, host: string): PullRequestActor | null { return { login: actor.kind === "local" ? actor.handle : actor.login, name: actor.kind === "local" ? actor.displayName : null, + ...(actor.kind === "local" && actor.bot === true ? { isBot: true } : {}), // GitCafe serves its own avatars by path, so a relative one is read against the host. avatarUrl: actor.avatarUrl === null ? null : new URL(actor.avatarUrl, `https://${host}`).href, }; @@ -264,6 +285,22 @@ export function toReviewers( }); } +export function toReviewerCandidates( + candidates: typeof GitCafeActorEntries.Type, + input: { + readonly requested: ReadonlySet; + readonly authorActorId: string; + readonly host: string; + }, +): ReadonlyArray { + return candidates.items.flatMap(({ id, actor }) => { + const reviewer = id === input.authorActorId ? null : toActor(actor, input.host); + return reviewer === null + ? [] + : [{ ...reviewer, id, kind: "user" as const, isRequested: input.requested.has(id) }]; + }); +} + function reviewerLogins(actor: Actor, host: string): ReadonlyArray { const reviewer = toActor(actor, host); if (reviewer === null) return []; @@ -293,7 +330,7 @@ export function toChangeRequest( reviewRequestLogins: (pull.reviewers ?? []).flatMap(({ actor }) => reviewerLogins(actor, target.host), ), - labels: pull.labels ?? [], + labels: (pull.labels ?? []).map(({ name, color }) => ({ name, color })), }; } @@ -313,7 +350,8 @@ export function toViewerPermissions(detail: GitCafePullDetail): PullRequestViewe comment: open, resolve: open, verdicts: review ? ["approve", "request-changes", "comment"] : [], - requestReviewers: false, + requestReviewers: edit && detail.state === "open", + labels: edit && detail.state === "open", editChangeRequest: edit, // GitCafe's only branch update moves a whole stack. updateMethods: [],