diff --git a/src/backend/pullrequests/index.test.ts b/src/backend/pullrequests/index.test.ts index 475302e..6d01244 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(); @@ -69,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" }, @@ -388,6 +393,12 @@ describe("getPullRequest", () => { state: "pending", }, ], + participants: [ + { + account: { uuid: "{eve}", displayName: "Eve", nickname: "eve" }, + state: "pending", + }, + ], }); }); @@ -412,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", + }, + ]); }); }); @@ -763,3 +814,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..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 = { @@ -419,6 +431,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; @@ -428,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), }; } @@ -444,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/index.ts b/src/commands/pullrequest/index.ts index 50bc08c..02ec253 100644 --- a/src/commands/pullrequest/index.ts +++ b/src/commands/pullrequest/index.ts @@ -3,6 +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 { runPullRequestReview } from "./review.ts"; import { runPullRequestView } from "./view.ts"; export function registerPullRequestCommands(program: Command): void { @@ -72,4 +73,18 @@ export function registerPullRequestCommands(program: Command): void { "Read comment body from a file ('-' for stdin)", ) .action(withRenderer(runPullRequestComment)); + + pr.command("review") + .description( + "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", + ) + .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 new file mode 100644 index 0000000..70d958e --- /dev/null +++ b/src/commands/pullrequest/review.ts @@ -0,0 +1,112 @@ +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; + approve?: boolean; + requestChanges?: boolean; + withdraw?: boolean; +}; + +/** + * 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, +): 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: "review", + }); + + 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 || + err instanceof PullRequestError + ) { + renderer.error(err.message); + process.exit(1); + } + throw err; + } +} + +/** + * 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; + } +} 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(", "); +}