From 1d1422709ef254de19e2bb5eaf7a3bb08f9939aa Mon Sep 17 00:00:00 2001 From: Nicolas Medda Date: Wed, 15 Apr 2026 12:00:32 +0000 Subject: [PATCH 1/3] BBC2-18 add bb pr approve/unapprove/request-changes/unrequest-changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four flat commands, each defaulting to the current branch's PR when no id is given: - bb pr approve / bb pr unapprove - bb pr request-changes / bb pr unrequest-changes Approve and request-changes are independent review states in Bitbucket — a reviewer can have an approval AND changes-requested from different users on the same PR simultaneously. The four commands wrap symmetric POST/DELETE pairs on /approve and /request-changes. Idempotency note: the spec doesn't document what re-approving an already-approved PR returns. We let any non-2xx propagate. The documented edge case — DELETE /approve returns 400 if the PR is already merged — surfaces as a clean PullRequestError; the command layer prints the message rather than a raw HTTP error. See docs/bb-notes.md → Approve / Unapprove / Request-changes for the endpoint details. --- src/backend/pullrequests/index.test.ts | 109 +++++++++++++++++++ src/backend/pullrequests/index.ts | 139 +++++++++++++++++++++++++ src/commands/pullrequest/index.ts | 50 +++++++++ src/commands/pullrequest/review.ts | 117 +++++++++++++++++++++ 4 files changed, 415 insertions(+) create mode 100644 src/commands/pullrequest/review.ts diff --git a/src/backend/pullrequests/index.test.ts b/src/backend/pullrequests/index.test.ts index 475302e..cb6c8e8 100644 --- a/src/backend/pullrequests/index.test.ts +++ b/src/backend/pullrequests/index.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test"; import { HttpResponse, http } from "msw"; import { BITBUCKET_BASE, server, setupMsw } from "../../test/msw/server.ts"; import { + approvePullRequest, createPullRequest, createPullRequestComment, findOpenPullRequestForBranch, @@ -11,6 +12,9 @@ import { type PullRequest, type PullRequestDetail, PullRequestError, + requestChangesOnPullRequest, + unapprovePullRequest, + withdrawRequestChanges, } from "./index.ts"; setupMsw(); @@ -763,3 +767,108 @@ describe("createPullRequestComment", () => { expect((err as PullRequestError).status).toBe(404); }); }); + +describe("review action endpoints (approve / unapprove / request-changes / unrequest-changes)", () => { + const APPROVE_PATH = (id: number) => + `${BITBUCKET_BASE}/repositories/ws/repo/pullrequests/${id}/approve`; + const REQUEST_CHANGES_PATH = (id: number) => + `${BITBUCKET_BASE}/repositories/ws/repo/pullrequests/${id}/request-changes`; + + test("approvePullRequest POSTs with no body", async () => { + let seenMethod: string | null = null; + let seenBody: string | null = null; + server.use( + http.post(APPROVE_PATH(42), async ({ request }) => { + seenMethod = request.method; + seenBody = await request.text(); + return HttpResponse.json( + { type: "participant", role: "REVIEWER", approved: true }, + { status: 200 }, + ); + }), + ); + + await approvePullRequest(creds, ref, 42); + + expect(seenMethod!).toBe("POST"); + expect(seenBody!).toBe(""); + }); + + test("unapprovePullRequest DELETEs and tolerates 204 no-content", async () => { + let seenMethod: string | null = null; + server.use( + http.delete(APPROVE_PATH(42), ({ request }) => { + seenMethod = request.method; + return new HttpResponse(null, { status: 204 }); + }), + ); + + await unapprovePullRequest(creds, ref, 42); + + expect(seenMethod!).toBe("DELETE"); + }); + + test("unapprovePullRequest surfaces 400 (PR already merged) cleanly", async () => { + // Documented case per the spec: DELETE /approve returns 400 if the + // PR has already been merged. We propagate the status so the command + // layer can show a clean message rather than a raw HTTP error. + server.use( + http.delete(APPROVE_PATH(42), () => + HttpResponse.json( + { + type: "error", + error: { message: "PR cannot be unapproved (merged)" }, + }, + { status: 400 }, + ), + ), + ); + + const err = await unapprovePullRequest(creds, ref, 42).catch((e) => e); + expect(err).toBeInstanceOf(PullRequestError); + expect((err as PullRequestError).status).toBe(400); + }); + + test("requestChangesOnPullRequest POSTs to /request-changes", async () => { + let seenMethod: string | null = null; + server.use( + http.post(REQUEST_CHANGES_PATH(42), ({ request }) => { + seenMethod = request.method; + return HttpResponse.json( + { type: "participant", role: "REVIEWER", state: "changes_requested" }, + { status: 200 }, + ); + }), + ); + + await requestChangesOnPullRequest(creds, ref, 42); + + expect(seenMethod!).toBe("POST"); + }); + + test("withdrawRequestChanges DELETEs to /request-changes", async () => { + let seenMethod: string | null = null; + server.use( + http.delete(REQUEST_CHANGES_PATH(42), ({ request }) => { + seenMethod = request.method; + return new HttpResponse(null, { status: 204 }); + }), + ); + + await withdrawRequestChanges(creds, ref, 42); + + expect(seenMethod!).toBe("DELETE"); + }); + + test("approvePullRequest surfaces 404 when the PR does not exist", async () => { + server.use( + http.post(APPROVE_PATH(99), () => + HttpResponse.json({ type: "error" }, { status: 404 }), + ), + ); + + const err = await approvePullRequest(creds, ref, 99).catch((e) => e); + expect(err).toBeInstanceOf(PullRequestError); + expect((err as PullRequestError).status).toBe(404); + }); +}); diff --git a/src/backend/pullrequests/index.ts b/src/backend/pullrequests/index.ts index 07441ac..06093a4 100644 --- a/src/backend/pullrequests/index.ts +++ b/src/backend/pullrequests/index.ts @@ -419,6 +419,145 @@ export async function createPullRequestComment( }; } +/** + * Records the authenticated user's review state on a PR. Approve and + * request-changes are two independent states; they map to symmetric + * POST/DELETE pairs on `/approve` and `/request-changes`. + * + * Idempotency is NOT documented by Bitbucket for any of these endpoints. + * We let non-2xx errors propagate; callers who need "re-running is a + * no-op" semantics should handle expected errors (e.g. a 409 conflict on + * re-approval) themselves. Smoke-test to find the actual behavior. + * + * `DELETE /approve` is documented to return `400` when the PR has already + * been merged — surface that at the command layer. + */ +async function postParticipantAction( + credentials: Credentials, + ref: { workspace: string; slug: string }, + pullRequestId: number, + action: "approve" | "request-changes", +): Promise { + const client = createBitbucketClient(credentials); + const { response } = + action === "approve" + ? await client.POST( + "/repositories/{workspace}/{repo_slug}/pullrequests/{pull_request_id}/approve", + { + params: { + path: { + workspace: ref.workspace, + repo_slug: ref.slug, + pull_request_id: pullRequestId, + }, + }, + }, + ) + : await client.POST( + "/repositories/{workspace}/{repo_slug}/pullrequests/{pull_request_id}/request-changes", + { + params: { + path: { + workspace: ref.workspace, + repo_slug: ref.slug, + pull_request_id: pullRequestId, + }, + }, + }, + ); + + if (!response.ok) { + throw new PullRequestError( + `Failed to ${action} pull request #${pullRequestId}: HTTP ${response.status}.`, + response.status, + ); + } +} + +async function deleteParticipantAction( + credentials: Credentials, + ref: { workspace: string; slug: string }, + pullRequestId: number, + action: "approve" | "request-changes", +): Promise { + const client = createBitbucketClient(credentials); + const { response } = + action === "approve" + ? await client.DELETE( + "/repositories/{workspace}/{repo_slug}/pullrequests/{pull_request_id}/approve", + { + params: { + path: { + workspace: ref.workspace, + repo_slug: ref.slug, + pull_request_id: pullRequestId, + }, + }, + }, + ) + : await client.DELETE( + "/repositories/{workspace}/{repo_slug}/pullrequests/{pull_request_id}/request-changes", + { + params: { + path: { + workspace: ref.workspace, + repo_slug: ref.slug, + pull_request_id: pullRequestId, + }, + }, + }, + ); + + if (!response.ok) { + throw new PullRequestError( + `Failed to withdraw ${action} on pull request #${pullRequestId}: HTTP ${response.status}.`, + response.status, + ); + } +} + +export async function approvePullRequest( + credentials: Credentials, + ref: { workspace: string; slug: string }, + pullRequestId: number, +): Promise { + await postParticipantAction(credentials, ref, pullRequestId, "approve"); +} + +export async function unapprovePullRequest( + credentials: Credentials, + ref: { workspace: string; slug: string }, + pullRequestId: number, +): Promise { + await deleteParticipantAction(credentials, ref, pullRequestId, "approve"); +} + +export async function requestChangesOnPullRequest( + credentials: Credentials, + ref: { workspace: string; slug: string }, + pullRequestId: number, +): Promise { + await postParticipantAction( + credentials, + ref, + pullRequestId, + "request-changes", + ); +} + +export async function withdrawRequestChanges( + credentials: Credentials, + ref: { workspace: string; slug: string }, + pullRequestId: number, +): Promise { + await deleteParticipantAction( + credentials, + ref, + pullRequestId, + "request-changes", + ); +} + function toPullRequestDetail(pr: RawPullRequest): PullRequestDetail { const base = toPullRequest(pr); const raw = pr as Record; diff --git a/src/commands/pullrequest/index.ts b/src/commands/pullrequest/index.ts index 50bc08c..5aac9f8 100644 --- a/src/commands/pullrequest/index.ts +++ b/src/commands/pullrequest/index.ts @@ -3,6 +3,12 @@ import { withRenderer } from "../../shared/renderer/commander.ts"; import { runPullRequestComment } from "./comment.ts"; import { runPullRequestCreate } from "./create.ts"; import { runPullRequestList } from "./list.ts"; +import { + runPullRequestApprove, + runPullRequestRequestChanges, + runPullRequestUnapprove, + runPullRequestUnrequestChanges, +} from "./review.ts"; import { runPullRequestView } from "./view.ts"; export function registerPullRequestCommands(program: Command): void { @@ -72,4 +78,48 @@ export function registerPullRequestCommands(program: Command): void { "Read comment body from a file ('-' for stdin)", ) .action(withRenderer(runPullRequestComment)); + + pr.command("approve") + .description( + "Approve a pull request (defaults to the PR for the current branch)", + ) + .argument("[id]", "Pull request number") + .option( + "-R, --repository ", + "Override repository detection", + ) + .action(withRenderer(runPullRequestApprove)); + + pr.command("unapprove") + .description( + "Withdraw your approval on a pull request (defaults to the PR for the current branch)", + ) + .argument("[id]", "Pull request number") + .option( + "-R, --repository ", + "Override repository detection", + ) + .action(withRenderer(runPullRequestUnapprove)); + + pr.command("request-changes") + .description( + "Mark a pull request as needing changes (defaults to the PR for the current branch)", + ) + .argument("[id]", "Pull request number") + .option( + "-R, --repository ", + "Override repository detection", + ) + .action(withRenderer(runPullRequestRequestChanges)); + + pr.command("unrequest-changes") + .description( + "Withdraw your request-for-changes on a pull request (defaults to the PR for the current branch)", + ) + .argument("[id]", "Pull request number") + .option( + "-R, --repository ", + "Override repository detection", + ) + .action(withRenderer(runPullRequestUnrequestChanges)); } diff --git a/src/commands/pullrequest/review.ts b/src/commands/pullrequest/review.ts new file mode 100644 index 0000000..b6867bf --- /dev/null +++ b/src/commands/pullrequest/review.ts @@ -0,0 +1,117 @@ +import { + approvePullRequest, + PullRequestError, + requestChangesOnPullRequest, + unapprovePullRequest, + withdrawRequestChanges, +} from "../../backend/pullrequests/index.ts"; +import { loadConfigOrExit } from "../../shared/config/index.ts"; +import type { Renderer } from "../../shared/renderer/index.ts"; +import { + RepositoryResolutionError, + resolveRepository, +} from "../../shared/repository/index.ts"; +import { resolveCurrentPullRequestId } from "./current.ts"; + +export type PullRequestReviewOptions = { + repository?: string; +}; + +type ReviewAction = ( + config: Awaited>, + ref: { workspace: string; slug: string }, + id: number, +) => Promise; + +async function runReviewAction( + renderer: Renderer, + idArg: string | undefined, + options: PullRequestReviewOptions, + action: ReviewAction, + commandName: string, + successMessage: (id: number) => string, +): Promise { + const config = await loadConfigOrExit(renderer); + + try { + const ref = await resolveRepository({ override: options.repository }); + const id = await resolveCurrentPullRequestId(idArg, { + renderer, + config, + ref, + commandName, + }); + + await action(config, ref, id); + renderer.message(successMessage(id)); + } catch (err) { + if ( + err instanceof RepositoryResolutionError || + err instanceof PullRequestError + ) { + renderer.error(err.message); + process.exit(1); + } + throw err; + } +} + +export function runPullRequestApprove( + renderer: Renderer, + idArg: string | undefined, + options: PullRequestReviewOptions, +): Promise { + return runReviewAction( + renderer, + idArg, + options, + approvePullRequest, + "approve", + (id) => `Approved pull request #${id}.`, + ); +} + +export function runPullRequestUnapprove( + renderer: Renderer, + idArg: string | undefined, + options: PullRequestReviewOptions, +): Promise { + return runReviewAction( + renderer, + idArg, + options, + unapprovePullRequest, + "unapprove", + (id) => `Withdrew approval on pull request #${id}.`, + ); +} + +export function runPullRequestRequestChanges( + renderer: Renderer, + idArg: string | undefined, + options: PullRequestReviewOptions, +): Promise { + return runReviewAction( + renderer, + idArg, + options, + requestChangesOnPullRequest, + "request-changes", + (id) => `Requested changes on pull request #${id}.`, + ); +} + +export function runPullRequestUnrequestChanges( + renderer: Renderer, + idArg: string | undefined, + options: PullRequestReviewOptions, +): Promise { + return runReviewAction( + renderer, + idArg, + options, + withdrawRequestChanges, + "unrequest-changes", + (id) => `Withdrew request-for-changes on pull request #${id}.`, + ); +} From 278502e3fc9df960838f14e7ac7801436d62f40e Mon Sep 17 00:00:00 2001 From: Nicolas Medda Date: Wed, 15 Apr 2026 12:37:08 +0000 Subject: [PATCH 2/3] BBC2-18 surface approvals in bb pr view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes to make 'I approved it, now where did it go?' visible: - Backend PullRequestDetail now carries participants[] alongside reviewers[]. reviewers[] stays strictly role=REVIEWER (formal reviewers, as before). participants[] covers role=PARTICIPANT — including ad-hoc approvers on PRs with no assigned reviewers (the Snyk auto-PR case). Pure commenters show up with state=pending. - bb pr view gets a single APPROVALS entry on the top block with a one-line summary: 'N approved, M changes requested' aggregating both arrays. Minimal addition; the larger REVIEWERS vs PARTICIPANTS section split is deferred to its own ticket along with the full pr view redesign. --- src/backend/pullrequests/index.test.ts | 49 +++++++++++++++++++++++++- src/backend/pullrequests/index.ts | 26 ++++++++++++++ src/commands/pullrequest/view.ts | 23 ++++++++++++ 3 files changed, 97 insertions(+), 1 deletion(-) diff --git a/src/backend/pullrequests/index.test.ts b/src/backend/pullrequests/index.test.ts index cb6c8e8..6d01244 100644 --- a/src/backend/pullrequests/index.test.ts +++ b/src/backend/pullrequests/index.test.ts @@ -73,7 +73,8 @@ function makePrDetail( user: { uuid: "{dave}", display_name: "Dave", nickname: "dave" }, state: null, }, - // non-reviewer participants (plain commenters) should be dropped + // PARTICIPANT with no review state (plain commenter) — surfaces + // in participants[] with state=pending, stays out of reviewers[] { role: "PARTICIPANT", user: { uuid: "{eve}", display_name: "Eve", nickname: "eve" }, @@ -392,6 +393,12 @@ describe("getPullRequest", () => { state: "pending", }, ], + participants: [ + { + account: { uuid: "{eve}", displayName: "Eve", nickname: "eve" }, + state: "pending", + }, + ], }); }); @@ -416,6 +423,46 @@ describe("getPullRequest", () => { const result = await getPullRequest(creds, ref, 42); expect(result.reviewers).toEqual([]); + expect(result.participants).toEqual([]); + }); + + test("surfaces ad-hoc approvers as participants (Snyk-style no-reviewer PR)", async () => { + // Snyk auto-PRs ship with no formal reviewers. When someone approves + // one, Bitbucket records them as role=PARTICIPANT with state=approved. + // We want that surfaced so the approver can see their own action + // landed. + server.use( + http.get(PR_DETAIL_PATH(42), () => + HttpResponse.json( + makePrDetail({ + participants: [ + { + role: "PARTICIPANT", + user: { + uuid: "{nico}", + display_name: "Nicolas", + nickname: "nicolas", + }, + state: "approved", + }, + ], + }), + ), + ), + ); + + const result = await getPullRequest(creds, ref, 42); + expect(result.reviewers).toEqual([]); + expect(result.participants).toEqual([ + { + account: { + uuid: "{nico}", + displayName: "Nicolas", + nickname: "nicolas", + }, + state: "approved", + }, + ]); }); }); diff --git a/src/backend/pullrequests/index.ts b/src/backend/pullrequests/index.ts index 06093a4..535401e 100644 --- a/src/backend/pullrequests/index.ts +++ b/src/backend/pullrequests/index.ts @@ -48,11 +48,23 @@ export type Reviewer = { state: ReviewState; }; +/** + * A non-reviewer participant on a PR. State is populated when the person + * has approved or requested changes without being on the formal reviewer + * list (e.g. someone approving a Snyk auto-PR with no assigned reviewers). + * Pure commenters appear with state="pending". + */ +export type Participant = { + account: PullRequestAuthor; + state: ReviewState; +}; + export type PullRequestDetail = PullRequest & { description: string; sourceBranch: string; destinationBranch: string; reviewers: Reviewer[]; + participants: Participant[]; }; export type ListPullRequestsOptions = { @@ -567,6 +579,7 @@ function toPullRequestDetail(pr: RawPullRequest): PullRequestDetail { sourceBranch: String(raw.source?.branch?.name ?? ""), destinationBranch: String(raw.destination?.branch?.name ?? ""), reviewers: toReviewers(raw.participants), + participants: toParticipants(raw.participants), }; } @@ -583,6 +596,19 @@ function toReviewers(raw: unknown): Reviewer[] { return out; } +function toParticipants(raw: unknown): Participant[] { + if (!Array.isArray(raw)) return []; + const out: Participant[] = []; + for (const p of raw as RawParticipant[]) { + const pp = p as Record; + if (pp.role !== "PARTICIPANT") continue; + const account = toAuthor(pp.user); + if (!account) continue; + out.push({ account, state: toReviewState(pp.state) }); + } + return out; +} + function toReviewState(raw: unknown): ReviewState { if (raw === "approved") return "approved"; if (raw === "changes_requested") return "changes_requested"; diff --git a/src/commands/pullrequest/view.ts b/src/commands/pullrequest/view.ts index d8c9fed..3d76f90 100644 --- a/src/commands/pullrequest/view.ts +++ b/src/commands/pullrequest/view.ts @@ -73,6 +73,11 @@ function render(renderer: Renderer, pr: PullRequestDetail): void { label: "BRANCH", value: (p) => `${p.sourceBranch} → ${p.destinationBranch}`, }, + { + label: "APPROVALS", + value: (p) => summarizeReview(p), + style: "muted", + }, { label: "CREATED", value: (p) => formatRelativeTime(p.createdOn), @@ -103,3 +108,21 @@ function render(renderer: Renderer, pr: PullRequestDetail): void { ); } } + +/** + * One-line approval summary covering both formal reviewers and ad-hoc + * participants (e.g. someone who approves a Snyk PR that has no reviewers). + * This is the minimum needed to confirm "my approval landed" without the + * full reviewers-vs-participants section split — that redesign is its own + * ticket. + */ +function summarizeReview(pr: PullRequestDetail): string { + const all = [...pr.reviewers, ...pr.participants]; + const approved = all.filter((p) => p.state === "approved").length; + const changes = all.filter((p) => p.state === "changes_requested").length; + if (approved === 0 && changes === 0) return "(none)"; + const parts: string[] = []; + if (approved > 0) parts.push(`${approved} approved`); + if (changes > 0) parts.push(`${changes} changes requested`); + return parts.join(", "); +} From 35989f84ffea45e6a6a98dc4e4801cb8341ab113 Mon Sep 17 00:00:00 2001 From: Nicolas Medda Date: Wed, 15 Apr 2026 13:10:35 +0000 Subject: [PATCH 3/3] BBC2-18 collapse review commands into unified bb pr review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the four flat commands (approve / unapprove / request-changes / unrequest-changes) with a single bb pr review taking mutually- exclusive flags: -a, --approve -r, --request-changes -w, --withdraw Matches gh's mental model of a single 'current review state' rather than Bitbucket's two-independent-toggles API. --approve and --request-changes implicitly clear the inverse state so the reviewer can flip cleanly; --withdraw clears whichever state is set. Backend functions unchanged — they remain the primitives the command layer composes. The command layer's secondary cleanup calls are best-effort (swallow PullRequestError) so a primary-action success isn't masked by an expected 'not currently in that state' failure on the cleanup DELETE. --- src/commands/pullrequest/index.ts | 49 ++--------- src/commands/pullrequest/review.ts | 137 ++++++++++++++--------------- 2 files changed, 73 insertions(+), 113 deletions(-) diff --git a/src/commands/pullrequest/index.ts b/src/commands/pullrequest/index.ts index 5aac9f8..02ec253 100644 --- a/src/commands/pullrequest/index.ts +++ b/src/commands/pullrequest/index.ts @@ -3,12 +3,7 @@ import { withRenderer } from "../../shared/renderer/commander.ts"; import { runPullRequestComment } from "./comment.ts"; import { runPullRequestCreate } from "./create.ts"; import { runPullRequestList } from "./list.ts"; -import { - runPullRequestApprove, - runPullRequestRequestChanges, - runPullRequestUnapprove, - runPullRequestUnrequestChanges, -} from "./review.ts"; +import { runPullRequestReview } from "./review.ts"; import { runPullRequestView } from "./view.ts"; export function registerPullRequestCommands(program: Command): void { @@ -79,47 +74,17 @@ export function registerPullRequestCommands(program: Command): void { ) .action(withRenderer(runPullRequestComment)); - pr.command("approve") + pr.command("review") .description( - "Approve a pull request (defaults to the PR for the current branch)", + "Submit a review on a pull request (defaults to the PR for the current branch)", ) .argument("[id]", "Pull request number") .option( "-R, --repository ", "Override repository detection", ) - .action(withRenderer(runPullRequestApprove)); - - pr.command("unapprove") - .description( - "Withdraw your approval on a pull request (defaults to the PR for the current branch)", - ) - .argument("[id]", "Pull request number") - .option( - "-R, --repository ", - "Override repository detection", - ) - .action(withRenderer(runPullRequestUnapprove)); - - pr.command("request-changes") - .description( - "Mark a pull request as needing changes (defaults to the PR for the current branch)", - ) - .argument("[id]", "Pull request number") - .option( - "-R, --repository ", - "Override repository detection", - ) - .action(withRenderer(runPullRequestRequestChanges)); - - pr.command("unrequest-changes") - .description( - "Withdraw your request-for-changes on a pull request (defaults to the PR for the current branch)", - ) - .argument("[id]", "Pull request number") - .option( - "-R, --repository ", - "Override repository detection", - ) - .action(withRenderer(runPullRequestUnrequestChanges)); + .option("-a, --approve", "Approve the pull request") + .option("-r, --request-changes", "Request changes on the pull request") + .option("-w, --withdraw", "Withdraw your current review state") + .action(withRenderer(runPullRequestReview)); } diff --git a/src/commands/pullrequest/review.ts b/src/commands/pullrequest/review.ts index b6867bf..70d958e 100644 --- a/src/commands/pullrequest/review.ts +++ b/src/commands/pullrequest/review.ts @@ -15,35 +15,77 @@ import { resolveCurrentPullRequestId } from "./current.ts"; export type PullRequestReviewOptions = { repository?: string; + approve?: boolean; + requestChanges?: boolean; + withdraw?: boolean; }; -type ReviewAction = ( - config: Awaited>, - ref: { workspace: string; slug: string }, - id: number, -) => Promise; - -async function runReviewAction( +/** + * Submits a review on a PR. Bitbucket's API exposes approve/request-changes + * as two independent mutable states; we hide that behind a single "current + * review state" mental model (approved / changes-requested / nil). Setting + * one state implicitly clears the other; --withdraw clears both. + */ +export async function runPullRequestReview( renderer: Renderer, idArg: string | undefined, options: PullRequestReviewOptions, - action: ReviewAction, - commandName: string, - successMessage: (id: number) => string, ): Promise { const config = await loadConfigOrExit(renderer); + const selected = [ + options.approve ? "--approve" : null, + options.requestChanges ? "--request-changes" : null, + options.withdraw ? "--withdraw" : null, + ].filter((v): v is string => v !== null); + + if (selected.length === 0) { + renderer.error( + "Specify one of --approve (-a), --request-changes (-r), or --withdraw (-w).", + ); + process.exit(1); + } + if (selected.length > 1) { + renderer.error( + `--approve, --request-changes, and --withdraw are mutually exclusive; got ${selected.join(", ")}.`, + ); + process.exit(1); + } + try { const ref = await resolveRepository({ override: options.repository }); const id = await resolveCurrentPullRequestId(idArg, { renderer, config, ref, - commandName, + commandName: "review", }); - await action(config, ref, id); - renderer.message(successMessage(id)); + if (options.approve) { + await approvePullRequest(config, ref, id); + // If the reviewer previously requested changes, drop that so their + // state is singular. Best-effort — the approve is what matters. + await swallow(() => withdrawRequestChanges(config, ref, id)); + renderer.message(`Approved pull request #${id}.`); + return; + } + + if (options.requestChanges) { + await requestChangesOnPullRequest(config, ref, id); + await swallow(() => unapprovePullRequest(config, ref, id)); + renderer.message(`Requested changes on pull request #${id}.`); + return; + } + + // --withdraw: clear whichever state is set. Both best-effort since we + // don't know the current state without an extra fetch; a DELETE on a + // state we're not in is harmless (Bitbucket noops or 4xx's — we + // swallow either way). + await Promise.all([ + swallow(() => unapprovePullRequest(config, ref, id)), + swallow(() => withdrawRequestChanges(config, ref, id)), + ]); + renderer.message(`Withdrew review on pull request #${id}.`); } catch (err) { if ( err instanceof RepositoryResolutionError || @@ -56,62 +98,15 @@ async function runReviewAction( } } -export function runPullRequestApprove( - renderer: Renderer, - idArg: string | undefined, - options: PullRequestReviewOptions, -): Promise { - return runReviewAction( - renderer, - idArg, - options, - approvePullRequest, - "approve", - (id) => `Approved pull request #${id}.`, - ); -} - -export function runPullRequestUnapprove( - renderer: Renderer, - idArg: string | undefined, - options: PullRequestReviewOptions, -): Promise { - return runReviewAction( - renderer, - idArg, - options, - unapprovePullRequest, - "unapprove", - (id) => `Withdrew approval on pull request #${id}.`, - ); -} - -export function runPullRequestRequestChanges( - renderer: Renderer, - idArg: string | undefined, - options: PullRequestReviewOptions, -): Promise { - return runReviewAction( - renderer, - idArg, - options, - requestChangesOnPullRequest, - "request-changes", - (id) => `Requested changes on pull request #${id}.`, - ); -} - -export function runPullRequestUnrequestChanges( - renderer: Renderer, - idArg: string | undefined, - options: PullRequestReviewOptions, -): Promise { - return runReviewAction( - renderer, - idArg, - options, - withdrawRequestChanges, - "unrequest-changes", - (id) => `Withdrew request-for-changes on pull request #${id}.`, - ); +/** + * Swallows PullRequestError so secondary cleanup calls don't mask the + * primary action's success. Other errors propagate. + */ +async function swallow(fn: () => Promise): Promise { + try { + await fn(); + } catch (err) { + if (err instanceof PullRequestError) return; + throw err; + } }