diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts index 8c55e786c7edd..268bbb30a7e94 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts @@ -1421,6 +1421,50 @@ layer("GitLabPullRequestCli.layer", (it) => { }), ); + it.effect("requests changes as a reviewer and reports what GitLab refused", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce(Effect.succeed(output(JSON.stringify({ username: "bilal" })))) + .mockReturnValueOnce( + Effect.succeed( + output(JSON.stringify({ data: { mergeRequestSetReviewers: { errors: [] } } })), + ), + ) + .mockReturnValueOnce( + Effect.succeed( + output( + JSON.stringify({ + data: { mergeRequestRequestChanges: { errors: ["Merge request is locked"] } }, + }), + ), + ), + ); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + + const error = yield* Effect.flip( + cli.submitReview({ + cwd: "/w", + repository: "acme/web", + number: 7, + verdict: "request-changes", + body: "Needs work", + comments: [{ path: "src/a.ts", position: { kind: "added", newLine: 4 }, body: "nit" }], + }), + ); + + expect(mockedExecute).toHaveBeenCalledTimes(3); + const target = { projectPath: "acme/web", iid: "7" }; + const setReviewers = JSON.parse(callAt(1).stdin ?? ""); + expect(setReviewers.query).toContain("operationMode: APPEND"); + expect(setReviewers.variables).toEqual({ ...target, reviewers: ["bilal"] }); + const requestChanges = JSON.parse(callAt(2).stdin ?? ""); + expect(requestChanges.query).toContain("mergeRequestRequestChanges"); + expect(requestChanges.variables).toEqual(target); + assert.strictEqual(error._tag, "GitLabMutationRefusedError"); + expect(error.detail).toBe("GitLab refused it: Merge request is locked"); + }), + ); + it.effect("removes an award by listing them and deleting the reader's own id", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce( diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts index f3a0c2cd391c4..0e7869324442a 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts @@ -51,6 +51,7 @@ import { decodeMergeRequestListJson, decodeMergeRequestFileStatsJson, decodeMergeRequestStatsJson, + decodeMutationErrorsJson, decodeNotesJson, decodeOwnAwardIdJson, decodePipelineStageChecksJson, @@ -204,6 +205,34 @@ export class GitLabDiffFileContentsUnavailableError extends Schema.TaggedError()( + "GitLabMutationRefusedError", + { + command: Schema.Literal("glab"), + cwd: Schema.String, + operation: Schema.String, + reasons: Schema.Array(Schema.String), + }, +) { + get detail(): string { + return `GitLab refused it: ${this.reasons.join("; ")}`; + } + + override get message(): string { + return `GitLab CLI failed in ${this.operation}: ${this.detail}`; + } +} + +const SET_SELF_AS_REVIEWER_MUTATION = `mutation($projectPath: ID!, $iid: String!, $reviewers: [String!]!) { + mergeRequestSetReviewers( + input: { projectPath: $projectPath, iid: $iid, reviewerUsernames: $reviewers, operationMode: APPEND } + ) { errors } +}`; + +const REQUEST_CHANGES_MUTATION = `mutation($projectPath: ID!, $iid: String!) { + mergeRequestRequestChanges(input: { projectPath: $projectPath, iid: $iid }) { errors } +}`; + export class GitLabBranchDeletionRefusedError extends Schema.TaggedError()( "GitLabBranchDeletionRefusedError", { @@ -266,7 +295,8 @@ export type GitLabPullRequestCliError = | GitLabDiffCommitParentUnavailableError | GitLabDiffFileContentsUnavailableError | GitLabDiffRefsUnavailableError - | GitLabViewerUnavailableError; + | GitLabViewerUnavailableError + | GitLabMutationRefusedError; /** GitLab's `per_page` limit. */ const MAX_PAGE_SIZE = 100; @@ -1452,6 +1482,28 @@ export const make = Effect.gen(function* () { }), ); + const graphqlMutation = (input: { + readonly cwd: string; + readonly host?: string | undefined; + readonly operation: string; + readonly query: string; + readonly variables: Record; + }) => + graphqlRead({ ...input, decode: decodeMutationErrorsJson }).pipe( + Effect.flatMap((reasons) => + reasons.length === 0 + ? Effect.void + : Effect.fail( + new GitLabMutationRefusedError({ + command: "glab", + cwd: input.cwd, + operation: input.operation, + reasons, + }), + ), + ), + ); + const graphqlListPage = (input: { readonly cwd: string; readonly host?: string | undefined; @@ -2335,10 +2387,29 @@ export const make = Effect.gen(function* () { submitReview: Effect.fn("GitLabPullRequestCli.submitReview")(function* (input) { const project = projectPath(input.repository); const mergeRequest = `projects/${project}/merge_requests/${input.number}`; - // GitLab has no pending review to attach comments to, so a review is replayed as the - // requests it is made of: the line comments, then the summary, then the verdict. A - // failure part-way therefore leaves what was already posted in place, which is why - // the verdict goes last — a half-sent review is never an approval. + // GitLab has no pending review, so a review is replayed as separate requests and a failure + // leaves earlier ones posted. Requesting changes (GitLab 18.9+) goes first so an older + // server fails before any comment lands; approval goes last so a half-sent review never + // approves. + if (input.verdict === "request-changes") { + // GitLab keeps the verdict on the reviewer, so the viewer joins the reviewers first. + const viewer = yield* viewerUsername(input); + const target = { projectPath: input.repository, iid: String(input.number) }; + yield* graphqlMutation({ + cwd: input.cwd, + host: input.host, + operation: "submitReview", + query: SET_SELF_AS_REVIEWER_MUTATION, + variables: { ...target, reviewers: [viewer] }, + }); + yield* graphqlMutation({ + cwd: input.cwd, + host: input.host, + operation: "submitReview", + query: REQUEST_CHANGES_MUTATION, + variables: target, + }); + } if (input.comments.length > 0) { const refs = yield* getDiffRefs(input); yield* Effect.forEach( diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts index d5e5793c68b8b..e2adb243b9703 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.test.ts @@ -143,7 +143,7 @@ describe("gitLabViewerPermissions", () => { ], comment: true, resolve: true, - verdicts: ["comment", "approve"], + verdicts: ["comment", "approve", "request-changes"], requestReviewers: true, labels: true, updateMethods: ["rebase"], @@ -155,7 +155,7 @@ describe("gitLabViewerPermissions", () => { actions: ["ready", "draft", "close", "reopen"], comment: true, resolve: true, - verdicts: ["comment", "approve"], + verdicts: ["comment", "approve", "request-changes"], requestReviewers: true, labels: false, }); diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts index ec3f46f0ac2a8..126ded60a8ec7 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts @@ -38,8 +38,7 @@ const CAPABILITIES: PullRequestCapabilities = { inlineComment: true, reply: true, resolve: true, - // GitLab has no requested-changes verdict. - verdicts: ["comment", "approve"], + verdicts: ["comment", "approve", "request-changes"], }, reviewers: { request: true, listCandidates: true }, labels: true, diff --git a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts index 770712c2397d0..18366b536786f 100644 --- a/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts +++ b/packages/source-control-gitlab/src/server/gitLabMergeRequestJson.ts @@ -1712,6 +1712,28 @@ export function decodeGraphqlListSupportJson(raw: string): Result.Result 17 || (major === 17 && minor >= 9)); } +const decodeMutationPayloads = decodeJsonResult( + Schema.Struct({ + data: Schema.Record( + Schema.String, + Schema.NullOr( + Schema.Struct({ errors: Schema.optional(Schema.NullOr(Schema.Array(Schema.String))) }), + ), + ), + }), +); + +/** GitLab answers a refused mutation with HTTP 200 and the reasons in each payload's `errors`. */ +export function decodeMutationErrorsJson( + raw: string, +): Result.Result, DecodeFailure> { + const decoded = decodeMutationPayloads(raw); + if (!Result.isSuccess(decoded)) return Result.fail(decoded.failure); + return Result.succeed( + Object.values(decoded.success.data).flatMap((payload) => payload?.errors ?? []), + ); +} + /** * Line counts for merge requests of one project. The REST listing carries no line counts, and * reading each diff to count them would cost a request per row, so GraphQL's per-merge-request