From b0b1e4ba595a0fa6b16917ceab2145f366fe8449 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 1 Jun 2026 02:17:44 -0700 Subject: [PATCH 01/12] feat(cli): add gh-backed PR write routes Mutation routes for title edit, close/reopen, draft toggle, merge, auto-merge (merge-queue enqueue/dequeue map to gh's --auto/--disable-auto), and reviewer add/remove, plus a collaborators read for the reviewer picker. Writes surface gh failures as 500s so the UI can toast them. Shared run/repo resolution is factored into pull-request-shared.ts. Covered by a route test asserting the exact gh argv per mutation. --- .../pull-request-mutations.routes.test.ts | 182 ++++++++++++++++++ packages/cli/src/github-mutations.ts | 136 +++++++++++++ .../cli/src/routes/pull-request-mutations.ts | 170 ++++++++++++++++ .../cli/src/routes/pull-request-shared.ts | 59 ++++++ packages/cli/src/routes/pull-request.ts | 68 +------ packages/cli/src/show.ts | 9 +- packages/types/src/pull-request.ts | 5 + 7 files changed, 563 insertions(+), 66 deletions(-) create mode 100644 packages/cli/src/__tests__/pull-request-mutations.routes.test.ts create mode 100644 packages/cli/src/github-mutations.ts create mode 100644 packages/cli/src/routes/pull-request-mutations.ts create mode 100644 packages/cli/src/routes/pull-request-shared.ts diff --git a/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts b/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts new file mode 100644 index 0000000..cb2f082 --- /dev/null +++ b/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts @@ -0,0 +1,182 @@ +import fs from "node:fs/promises"; +import http from "node:http"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { closeDb, getDb } from "../db/client.js"; +import { chapterRun } from "../db/schema/index.js"; +import { pullRequestMutationRoutes } from "../routes/pull-request-mutations.js"; +import { SCOPE_KIND } from "../schema.js"; +import { LOOPBACK_HOST, type ServerHandle, startServer } from "../server.js"; + +let tmpDir: string; +let dbPath: string; +let webDist: string; +let repoRoot: string; +let binDir: string; +let logFile: string; +let originalPath: string | undefined; +const handles: ServerHandle[] = []; + +const SHA = "a".repeat(40); +const GITHUB_ORIGIN = "git@github.com:owner/repo.git"; + +beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "stage-cli-pr-mut-")); + dbPath = path.join(tmpDir, "db.sqlite"); + webDist = path.join(tmpDir, "web-dist"); + repoRoot = path.join(tmpDir, "repo"); + binDir = path.join(tmpDir, "bin"); + logFile = path.join(tmpDir, "gh-args.log"); + await fs.mkdir(webDist); + await fs.writeFile(path.join(webDist, "index.html"), ""); + await fs.mkdir(repoRoot); + await fs.mkdir(binDir); + // Fake gh that records its argv (one line per invocation) and succeeds. + await fs.writeFile(path.join(binDir, "gh"), `#!/bin/sh\necho "$@" >> "${logFile}"\n`); + await fs.chmod(path.join(binDir, "gh"), 0o755); + originalPath = process.env.PATH; + process.env.PATH = `${binDir}${path.delimiter}${originalPath ?? ""}`; + closeDb(); +}); + +afterEach(async () => { + while (handles.length > 0) { + const h = handles.pop(); + if (h) await h.close(); + } + closeDb(); + process.env.PATH = originalPath; + await fs.rm(tmpDir, { recursive: true, force: true }); +}); + +function insertRun(): string { + const db = getDb({ dbPath }); + const [row] = db + .insert(chapterRun) + .values({ + repoRoot, + originUrl: GITHUB_ORIGIN, + scopeKind: SCOPE_KIND.COMMITTED, + workingTreeRef: null, + baseSha: SHA, + headSha: SHA, + mergeBaseSha: SHA, + generatedAt: new Date(), + }) + .returning({ id: chapterRun.id }) + .all(); + if (!row) throw new Error("seed: chapter_run insert returned no row"); + return row.id; +} + +async function start(): Promise { + const db = getDb({ dbPath }); + const handle = await startServer({ webDistPath: webDist, routes: pullRequestMutationRoutes(db) }); + handles.push(handle); + return handle.port; +} + +function send( + port: number, + method: string, + p: string, + body: unknown, +): Promise<{ status: number; body: string }> { + return new Promise((resolve, reject) => { + const payload = JSON.stringify(body); + const req = http.request( + { + hostname: LOOPBACK_HOST, + port, + method, + path: p, + agent: false, + headers: { + "Content-Type": "application/json", + "Content-Length": Buffer.byteLength(payload), + }, + }, + (res) => { + const chunks: Buffer[] = []; + res.on("data", (c: Buffer) => chunks.push(c)); + res.on("end", () => + resolve({ status: res.statusCode ?? 0, body: Buffer.concat(chunks).toString("utf8") }), + ); + }, + ); + req.on("error", reject); + req.end(payload); + }); +} + +async function ghArgs(): Promise { + try { + return (await fs.readFile(logFile, "utf8")).trim().split("\n").filter(Boolean); + } catch { + return []; + } +} + +describe("pull-request mutation API", () => { + it("invokes `gh pr edit --title` for a title change", async () => { + const runId = insertRun(); + const res = await send(await start(), "PATCH", `/api/runs/${runId}/pull-request/title`, { + number: 7, + title: "New title", + }); + expect(res.status).toBe(200); + expect(await ghArgs()).toEqual(["pr edit 7 --title New title"]); + }); + + it("invokes `gh pr merge --squash --match-head-commit` for a squash merge", async () => { + const runId = insertRun(); + const res = await send(await start(), "POST", `/api/runs/${runId}/pull-request/merge`, { + number: 7, + mergeMethod: "SQUASH", + expectedHeadOid: SHA, + }); + expect(res.status).toBe(200); + expect(await ghArgs()).toEqual([`pr merge 7 --squash --match-head-commit ${SHA}`]); + }); + + it("maps auto-merge enable/disable onto gh's --auto/--disable-auto", async () => { + const runId = insertRun(); + const port = await start(); + await send(port, "POST", `/api/runs/${runId}/pull-request/auto-merge`, { + number: 7, + enabled: true, + mergeMethod: "MERGE", + }); + await send(port, "POST", `/api/runs/${runId}/pull-request/auto-merge`, { + number: 7, + enabled: false, + }); + expect(await ghArgs()).toEqual(["pr merge 7 --auto --merge", "pr merge 7 --disable-auto"]); + }); + + it("adds and removes reviewers via gh pr edit", async () => { + const runId = insertRun(); + const port = await start(); + await send(port, "POST", `/api/runs/${runId}/pull-request/reviewers`, { + number: 7, + reviewers: ["alice", "bob"], + }); + await send(port, "DELETE", `/api/runs/${runId}/pull-request/reviewers`, { + number: 7, + reviewers: ["bob"], + }); + expect(await ghArgs()).toEqual([ + "pr edit 7 --add-reviewer alice,bob", + "pr edit 7 --remove-reviewer bob", + ]); + }); + + it("rejects an invalid body", async () => { + const runId = insertRun(); + const res = await send(await start(), "POST", `/api/runs/${runId}/pull-request/draft`, { + number: -1, + }); + expect(res.status).toBe(400); + }); +}); diff --git a/packages/cli/src/github-mutations.ts b/packages/cli/src/github-mutations.ts new file mode 100644 index 0000000..6862a23 --- /dev/null +++ b/packages/cli/src/github-mutations.ts @@ -0,0 +1,136 @@ +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; +import { + PULL_REQUEST_MERGE_METHOD, + type PullRequestMergeMethod, +} from "@stagereview/types/pull-request"; +import type { GitHubRepo } from "./github/index.js"; + +const execFileAsync = promisify(execFile); + +/** + * Run a `gh` write command in `repoRoot`. Unlike the read adapters in + * github.ts (which swallow errors to null), writes surface failures so the UI + * can toast them — the user explicitly asked to mutate their PR. + */ +async function ghWrite(args: string[], repoRoot: string): Promise { + try { + await execFileAsync("gh", args, { cwd: repoRoot, encoding: "utf8" }); + } catch (err: unknown) { + const stderr = + typeof err === "object" && err !== null && "stderr" in err && typeof err.stderr === "string" + ? err.stderr.trim() + : ""; + throw new Error(stderr || (err instanceof Error ? err.message : "gh command failed")); + } +} + +const MERGE_METHOD_FLAG: Record = { + [PULL_REQUEST_MERGE_METHOD.MERGE]: "--merge", + [PULL_REQUEST_MERGE_METHOD.SQUASH]: "--squash", + [PULL_REQUEST_MERGE_METHOD.REBASE]: "--rebase", +}; + +export function editTitle(repoRoot: string, number: number, title: string): Promise { + return ghWrite(["pr", "edit", String(number), "--title", title], repoRoot); +} + +export function closePullRequest(repoRoot: string, number: number): Promise { + return ghWrite(["pr", "close", String(number)], repoRoot); +} + +export function reopenPullRequest(repoRoot: string, number: number): Promise { + return ghWrite(["pr", "reopen", String(number)], repoRoot); +} + +export function setDraft(repoRoot: string, number: number, draft: boolean): Promise { + // `gh pr ready` marks ready; `--undo` converts back to draft. + const args = ["pr", "ready", String(number)]; + if (draft) args.push("--undo"); + return ghWrite(args, repoRoot); +} + +export function mergePullRequest( + repoRoot: string, + number: number, + mergeMethod: PullRequestMergeMethod, + expectedHeadOid?: string, +): Promise { + const args = ["pr", "merge", String(number), MERGE_METHOD_FLAG[mergeMethod]]; + if (expectedHeadOid) args.push("--match-head-commit", expectedHeadOid); + return ghWrite(args, repoRoot); +} + +/** + * Enable/disable auto-merge. On merge-queue repos `gh pr merge --auto` enqueues + * when ready, so the UI's enqueue/dequeue toggles map onto this too. + */ +export function setAutoMerge( + repoRoot: string, + number: number, + enabled: boolean, + mergeMethod?: PullRequestMergeMethod, +): Promise { + if (!enabled) return ghWrite(["pr", "merge", String(number), "--disable-auto"], repoRoot); + const args = ["pr", "merge", String(number), "--auto"]; + if (mergeMethod) args.push(MERGE_METHOD_FLAG[mergeMethod]); + return ghWrite(args, repoRoot); +} + +export function addReviewers(repoRoot: string, number: number, logins: string[]): Promise { + return ghWrite(["pr", "edit", String(number), "--add-reviewer", logins.join(",")], repoRoot); +} + +export function removeReviewers(repoRoot: string, number: number, logins: string[]): Promise { + return ghWrite(["pr", "edit", String(number), "--remove-reviewer", logins.join(",")], repoRoot); +} + +const COLLABORATOR_FIELDS = "login,type,avatar_url"; + +interface Collaborator { + login: string; + avatar_url: string; + type: string; +} + +/** Repo collaborators eligible as reviewers, for the reviewer picker. */ +export async function listCollaborators( + repoRoot: string, + repo: GitHubRepo, +): Promise { + try { + const { stdout } = await execFileAsync( + "gh", + [ + "api", + `repos/${repo.owner}/${repo.repo}/collaborators`, + "--paginate", + "--jq", + `[.[] | {${COLLABORATOR_FIELDS}}]`, + ], + { cwd: repoRoot, encoding: "utf8", maxBuffer: 10 * 1024 * 1024 }, + ); + // --paginate with --jq emits one JSON array per page; concat them. + const collaborators: Collaborator[] = []; + for (const line of stdout.split("\n")) { + const trimmed = line.trim(); + if (!trimmed) continue; + const page: unknown = JSON.parse(trimmed); + if (Array.isArray(page)) { + for (const c of page) { + if ( + c && + typeof c.login === "string" && + typeof c.avatar_url === "string" && + typeof c.type === "string" + ) { + collaborators.push({ login: c.login, avatar_url: c.avatar_url, type: c.type }); + } + } + } + } + return collaborators; + } catch { + return []; + } +} diff --git a/packages/cli/src/routes/pull-request-mutations.ts b/packages/cli/src/routes/pull-request-mutations.ts new file mode 100644 index 0000000..35990d7 --- /dev/null +++ b/packages/cli/src/routes/pull-request-mutations.ts @@ -0,0 +1,170 @@ +import { + type CollaboratorsResponse, + PULL_REQUEST_MERGE_METHOD, +} from "@stagereview/types/pull-request"; +import { z } from "zod"; +import type { StageDb } from "../db/client.js"; +import { + addReviewers, + closePullRequest, + editTitle, + listCollaborators, + mergePullRequest, + removeReviewers, + reopenPullRequest, + setAutoMerge, + setDraft, +} from "../github-mutations.js"; +import type { Route, RouteHandler } from "../server.js"; +import { readJsonBody, writeJson } from "./json.js"; +import { requireRepo, resolveRun } from "./pull-request-shared.js"; + +type Req = Parameters[0]; +type Res = Parameters[1]; + +const numberField = z.number().int().positive(); +const mergeMethod = z.enum(PULL_REQUEST_MERGE_METHOD); + +const titleInput = z.object({ number: numberField, title: z.string().min(1) }); +const numberInput = z.object({ number: numberField }); +const draftInput = z.object({ number: numberField, draft: z.boolean() }); +const mergeInput = z.object({ + number: numberField, + mergeMethod, + expectedHeadOid: z.string().optional(), +}); +const autoMergeInput = z.object({ + number: numberField, + enabled: z.boolean(), + mergeMethod: mergeMethod.optional(), +}); +const reviewersInput = z.object({ number: numberField, reviewers: z.array(z.string()).min(1) }); + +async function parseBody(req: Req, res: Res, schema: z.ZodType): Promise { + const parsed = schema.safeParse(await readJsonBody(req)); + if (!parsed.success) { + writeJson(res, 400, { error: "Invalid request body" }); + return null; + } + return parsed.data; +} + +/** Run a gh write, surfacing failures as a 500 so the UI can toast the message. */ +async function runMutation(res: Res, fn: () => Promise): Promise { + try { + await fn(); + writeJson(res, 200, { ok: true }); + } catch (err) { + writeJson(res, 500, { error: err instanceof Error ? err.message : String(err) }); + } +} + +export function pullRequestMutationRoutes(db: StageDb): Route[] { + return [ + { + method: "PATCH", + pattern: "/api/runs/:runId/pull-request/title", + handler: async (req, res, params) => { + const run = resolveRun(db, params, res); + if (!run) return; + const input = await parseBody(req, res, titleInput); + if (!input) return; + await runMutation(res, () => editTitle(run.repoRoot, input.number, input.title)); + }, + }, + { + method: "POST", + pattern: "/api/runs/:runId/pull-request/close", + handler: async (req, res, params) => { + const run = resolveRun(db, params, res); + if (!run) return; + const input = await parseBody(req, res, numberInput); + if (!input) return; + await runMutation(res, () => closePullRequest(run.repoRoot, input.number)); + }, + }, + { + method: "POST", + pattern: "/api/runs/:runId/pull-request/reopen", + handler: async (req, res, params) => { + const run = resolveRun(db, params, res); + if (!run) return; + const input = await parseBody(req, res, numberInput); + if (!input) return; + await runMutation(res, () => reopenPullRequest(run.repoRoot, input.number)); + }, + }, + { + method: "POST", + pattern: "/api/runs/:runId/pull-request/draft", + handler: async (req, res, params) => { + const run = resolveRun(db, params, res); + if (!run) return; + const input = await parseBody(req, res, draftInput); + if (!input) return; + await runMutation(res, () => setDraft(run.repoRoot, input.number, input.draft)); + }, + }, + { + method: "POST", + pattern: "/api/runs/:runId/pull-request/merge", + handler: async (req, res, params) => { + const run = resolveRun(db, params, res); + if (!run) return; + const input = await parseBody(req, res, mergeInput); + if (!input) return; + await runMutation(res, () => + mergePullRequest(run.repoRoot, input.number, input.mergeMethod, input.expectedHeadOid), + ); + }, + }, + { + method: "POST", + pattern: "/api/runs/:runId/pull-request/auto-merge", + handler: async (req, res, params) => { + const run = resolveRun(db, params, res); + if (!run) return; + const input = await parseBody(req, res, autoMergeInput); + if (!input) return; + await runMutation(res, () => + setAutoMerge(run.repoRoot, input.number, input.enabled, input.mergeMethod), + ); + }, + }, + { + method: "POST", + pattern: "/api/runs/:runId/pull-request/reviewers", + handler: async (req, res, params) => { + const run = resolveRun(db, params, res); + if (!run) return; + const input = await parseBody(req, res, reviewersInput); + if (!input) return; + await runMutation(res, () => addReviewers(run.repoRoot, input.number, input.reviewers)); + }, + }, + { + method: "DELETE", + pattern: "/api/runs/:runId/pull-request/reviewers", + handler: async (req, res, params) => { + const run = resolveRun(db, params, res); + if (!run) return; + const input = await parseBody(req, res, reviewersInput); + if (!input) return; + await runMutation(res, () => removeReviewers(run.repoRoot, input.number, input.reviewers)); + }, + }, + { + method: "GET", + pattern: "/api/runs/:runId/pull-request/collaborators", + handler: async (_req, res, params) => { + const run = resolveRun(db, params, res); + if (!run) return; + const repo = requireRepo(run, res); + if (!repo) return; + const collaborators = await listCollaborators(run.repoRoot, repo); + const body: CollaboratorsResponse = { collaborators }; + writeJson(res, 200, body); + }, + }, + ]; +} diff --git a/packages/cli/src/routes/pull-request-shared.ts b/packages/cli/src/routes/pull-request-shared.ts new file mode 100644 index 0000000..8faabb5 --- /dev/null +++ b/packages/cli/src/routes/pull-request-shared.ts @@ -0,0 +1,59 @@ +import path from "node:path"; +import { eq } from "drizzle-orm"; +import type { StageDb } from "../db/client.js"; +import { chapterRun } from "../db/schema/index.js"; +import { type GitHubRepo, parseGitHubRepo } from "../github/index.js"; +import type { RouteHandler, RouteParams } from "../server.js"; +import { writeJson } from "./json.js"; + +type Res = Parameters[1]; +type Req = Parameters[0]; + +export interface RunRepo { + repoRoot: string; + originUrl: string | null; +} + +/** Resolve a run's repo context, writing the matching error response on failure. */ +export function resolveRun(db: StageDb, params: RouteParams, res: Res): RunRepo | null { + const runId = params.runId; + if (!runId) { + writeJson(res, 400, { error: "Missing runId" }); + return null; + } + const [run] = db.select().from(chapterRun).where(eq(chapterRun.id, runId)).limit(1).all(); + if (!run) { + writeJson(res, 404, { error: `Run ${runId} not found` }); + return null; + } + const repoRoot = run.repoRoot; + if (!path.isAbsolute(repoRoot) || repoRoot.split(path.sep).includes("..")) { + writeJson(res, 500, { + error: "Run repoRoot is not an absolute path or contains traversal segments", + }); + return null; + } + return { repoRoot, originUrl: run.originUrl }; +} + +export function requireRepo(run: RunRepo, res: Res): GitHubRepo | null { + const repo = parseGitHubRepo(run.originUrl); + if (!repo) { + writeJson(res, 404, { error: "Run is not associated with a GitHub remote" }); + return null; + } + return repo; +} + +export function query(req: Req, key: string): string | null { + const url = req.url ?? ""; + const qIdx = url.indexOf("?"); + if (qIdx < 0) return null; + return new URLSearchParams(url.slice(qIdx + 1)).get(key); +} + +export function parseNumber(value: string | null): number | null { + if (value === null) return null; + const n = Number(value); + return Number.isInteger(n) && n > 0 ? n : null; +} diff --git a/packages/cli/src/routes/pull-request.ts b/packages/cli/src/routes/pull-request.ts index d6183d6..6bcded8 100644 --- a/packages/cli/src/routes/pull-request.ts +++ b/packages/cli/src/routes/pull-request.ts @@ -1,76 +1,14 @@ -import path from "node:path"; import type { ChecksResponse, MergeStatusResponse, PullRequestResponse, ReviewsResponse, } from "@stagereview/types/pull-request"; -import { eq } from "drizzle-orm"; import type { StageDb } from "../db/client.js"; -import { chapterRun } from "../db/schema/index.js"; -import { - type GitHubRepo, - getChecks, - getMergeStatus, - getPullRequest, - getReviews, - parseGitHubRepo, -} from "../github/index.js"; -import type { Route, RouteHandler, RouteParams } from "../server.js"; +import { getChecks, getMergeStatus, getPullRequest, getReviews } from "../github/index.js"; +import type { Route } from "../server.js"; import { writeJson } from "./json.js"; - -interface RunRepo { - repoRoot: string; - originUrl: string | null; -} - -/** Resolve a run's repo context, writing the matching error response on failure. */ -function resolveRun( - db: StageDb, - params: RouteParams, - res: Parameters[1], -): RunRepo | null { - const runId = params.runId; - if (!runId) { - writeJson(res, 400, { error: "Missing runId" }); - return null; - } - const [run] = db.select().from(chapterRun).where(eq(chapterRun.id, runId)).limit(1).all(); - if (!run) { - writeJson(res, 404, { error: `Run ${runId} not found` }); - return null; - } - const repoRoot = run.repoRoot; - if (!path.isAbsolute(repoRoot) || repoRoot.split(path.sep).includes("..")) { - writeJson(res, 500, { - error: "Run repoRoot is not an absolute path or contains traversal segments", - }); - return null; - } - return { repoRoot, originUrl: run.originUrl }; -} - -function requireRepo(run: RunRepo, res: Parameters[1]): GitHubRepo | null { - const repo = parseGitHubRepo(run.originUrl); - if (!repo) { - writeJson(res, 404, { error: "Run is not associated with a GitHub remote" }); - return null; - } - return repo; -} - -function query(req: Parameters[0], key: string): string | null { - const url = req.url ?? ""; - const qIdx = url.indexOf("?"); - if (qIdx < 0) return null; - return new URLSearchParams(url.slice(qIdx + 1)).get(key); -} - -function parseNumber(value: string | null): number | null { - if (value === null) return null; - const n = Number(value); - return Number.isInteger(n) && n > 0 ? n : null; -} +import { parseNumber, query, requireRepo, resolveRun } from "./pull-request-shared.js"; const SHA_RE = /^[0-9a-f]{40}$/i; diff --git a/packages/cli/src/show.ts b/packages/cli/src/show.ts index 0479849..ff3847c 100644 --- a/packages/cli/src/show.ts +++ b/packages/cli/src/show.ts @@ -8,6 +8,7 @@ import { filterFilesForLlm, loadStageIgnore } from "./filter-files.js"; import { type ResolveScopeOptions, readRepoContext, readRepoRoot, resolveScope } from "./git.js"; import { diffRoutes } from "./routes/diff.js"; import { pullRequestRoutes } from "./routes/pull-request.js"; +import { pullRequestMutationRoutes } from "./routes/pull-request-mutations.js"; import { runRoutes } from "./routes/runs.js"; import { viewStateRoutes } from "./routes/view-state.js"; import { insertChaptersFile } from "./runs/import-chapters.js"; @@ -34,7 +35,13 @@ export async function show( const { runId } = insertChaptersFile(db, chaptersFile, readRepoContext()); const handle = await startServer({ - routes: [...runRoutes(db), ...viewStateRoutes(db), ...diffRoutes(db), ...pullRequestRoutes(db)], + routes: [ + ...runRoutes(db), + ...viewStateRoutes(db), + ...diffRoutes(db), + ...pullRequestRoutes(db), + ...pullRequestMutationRoutes(db), + ], }); const { port } = handle; const url = `http://${LOOPBACK_HOST}:${port}/runs/${encodeURIComponent(runId)}`; diff --git a/packages/types/src/pull-request.ts b/packages/types/src/pull-request.ts index a0b6f9d..5851c0a 100644 --- a/packages/types/src/pull-request.ts +++ b/packages/types/src/pull-request.ts @@ -200,6 +200,11 @@ export const ReviewsResponseSchema = z.object({ }); export type ReviewsResponse = z.infer; +export const CollaboratorsResponseSchema = z.object({ + collaborators: z.array(ReviewUserSchema), +}); +export type CollaboratorsResponse = z.infer; + // ─── Merge status ───────────────────────────────────────────────────────────── export const MergeQueueEntrySchema = z.object({ From 1a7f164fa62f23d3519b52d8affcb401a2e4420c Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 1 Jun 2026 02:17:45 -0700 Subject: [PATCH 02/12] feat(web): restore full interactivity on the vendored PR header Upgrade each read-only PR-header control to its full hosted version: inline title edit, status dropdown (close/reopen/draft) with close confirmation, merge-status controls (merge + auto-merge), and reviewer add/remove with collaborator search. Vendored components keep hosted markup; their oRPC mutations are swapped for CLI-native react-query hooks backed by the new gh write routes. Adds the alert-dialog primitive. --- packages/web/package.json | 1 + .../components/pull-request/merge-status.tsx | 272 ++++++++++++++ .../pull-request/pull-request-header.tsx | 332 ++++++++++++------ .../pull-request/pull-request-status.tsx | 152 ++++++++ .../src/components/pull-request/reviewers.tsx | 268 +++++++++++++- .../web/src/components/ui/alert-dialog.tsx | 132 +++++++ .../web/src/lib/pull-request-mutations.ts | 126 +++++++ .../lib/use-pull-request-status-actions.ts | 68 ++++ packages/web/src/lib/use-pull-request.ts | 13 + packages/web/src/lib/use-reviewer-manager.ts | 126 +++++++ pnpm-lock.yaml | 3 + 11 files changed, 1371 insertions(+), 122 deletions(-) create mode 100644 packages/web/src/components/pull-request/merge-status.tsx create mode 100644 packages/web/src/components/pull-request/pull-request-status.tsx create mode 100644 packages/web/src/components/ui/alert-dialog.tsx create mode 100644 packages/web/src/lib/pull-request-mutations.ts create mode 100644 packages/web/src/lib/use-pull-request-status-actions.ts create mode 100644 packages/web/src/lib/use-reviewer-manager.ts diff --git a/packages/web/package.json b/packages/web/package.json index ee2325d..d76a705 100644 --- a/packages/web/package.json +++ b/packages/web/package.json @@ -12,6 +12,7 @@ }, "dependencies": { "@pierre/diffs": "^1.0.11", + "@radix-ui/react-alert-dialog": "^1.1.15", "@radix-ui/react-avatar": "^1.1.10", "@radix-ui/react-checkbox": "^1.3.3", "@radix-ui/react-collapsible": "^1.1.12", diff --git a/packages/web/src/components/pull-request/merge-status.tsx b/packages/web/src/components/pull-request/merge-status.tsx new file mode 100644 index 0000000..c08fad3 --- /dev/null +++ b/packages/web/src/components/pull-request/merge-status.tsx @@ -0,0 +1,272 @@ +import { + type MergeStatusInfo, + PULL_REQUEST_MERGE_METHOD, + type PullRequestMergeMethod, +} from "@stagereview/types/pull-request"; +import { useMutation } from "@tanstack/react-query"; +import { Check, ChevronDown, GitMerge, Loader2 } from "lucide-react"; +import { useState } from "react"; +import { Button } from "@/components/ui/button"; +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuItem, + DropdownMenuTrigger, +} from "@/components/ui/dropdown-menu"; +import { Popover, PopoverContent, PopoverTrigger } from "@/components/ui/popover"; +import { toast } from "@/components/ui/sonner"; +import { Switch } from "@/components/ui/switch"; +import { usePullRequestContext } from "@/lib/pull-request-context"; +import { + dequeueMutationOptions, + enqueueMutationOptions, + mergeMutationOptions, + setAutoMergeMutationOptions, + useInvalidatePullRequest, +} from "@/lib/pull-request-mutations"; +import { cn } from "@/lib/utils"; +import { + getMergeStatusSummary, + MERGE_STATUS, + type MergeStatusSummary, +} from "./merge-status-summary"; + +const MERGE_METHOD_LABELS: Record = { + [PULL_REQUEST_MERGE_METHOD.SQUASH]: "Squash and merge", + [PULL_REQUEST_MERGE_METHOD.MERGE]: "Merge pull request", + [PULL_REQUEST_MERGE_METHOD.REBASE]: "Rebase and merge", +}; + +function MergeActions({ + mergeInfo, + summary, + owner, + repo, + number, + headSha, +}: { + mergeInfo: MergeStatusInfo; + summary: MergeStatusSummary; + owner: string; + repo: string; + number: number; + headSha: string; +}) { + const { runId } = usePullRequestContext(); + const invalidateAfterMutation = useInvalidatePullRequest(runId); + const [mergeMethod, setMergeMethod] = useState( + () => mergeInfo.allowedMergeMethods[0] ?? PULL_REQUEST_MERGE_METHOD.MERGE, + ); + + const mergeMutation = useMutation({ + ...mergeMutationOptions(runId), + onSuccess: () => { + toast.success("Pull request merged"); + invalidateAfterMutation(); + }, + onError: (error) => { + toast.error(error instanceof Error ? error.message : "Failed to merge pull request"); + }, + }); + + const enqueueMutation = useMutation({ + ...enqueueMutationOptions(runId), + onError: (error) => { + toast.error(error instanceof Error ? error.message : "Failed to add to merge queue"); + }, + onSettled: invalidateAfterMutation, + }); + + const autoMergeMutation = useMutation({ + ...setAutoMergeMutationOptions(runId), + onError: (error) => { + toast.error(error instanceof Error ? error.message : "Failed to update auto-merge"); + }, + onSettled: invalidateAfterMutation, + }); + + const dequeueMutation = useMutation({ + ...dequeueMutationOptions(runId), + onError: (error) => { + toast.error(error instanceof Error ? error.message : "Failed to remove from merge queue"); + }, + onSettled: invalidateAfterMutation, + }); + + const anyPending = + mergeMutation.isPending || + enqueueMutation.isPending || + autoMergeMutation.isPending || + dequeueMutation.isPending; + + const optimisticAutoMergeEnabled = + autoMergeMutation.isPending && autoMergeMutation.variables + ? autoMergeMutation.variables.enabled + : mergeInfo.autoMergeEnabled; + const optimisticInMergeQueue = enqueueMutation.isPending + ? true + : dequeueMutation.isPending + ? false + : mergeInfo.isInMergeQueue; + + const isReady = summary.status === MERGE_STATUS.READY; + + if (isReady && !mergeInfo.isMergeQueueEnabled && mergeInfo.allowedMergeMethods.length > 0) { + return ( +
+
+ + {mergeInfo.allowedMergeMethods.length > 1 && ( + + + + + + {mergeInfo.allowedMergeMethods.map((method) => ( + setMergeMethod(method)}> + + {MERGE_METHOD_LABELS[method]} + + ))} + + + )} +
+
+ ); + } + + if (mergeInfo.isMergeQueueEnabled) { + const isMergeWhenReady = optimisticInMergeQueue || optimisticAutoMergeEnabled; + return ( +
+ + {anyPending ? "Updating…" : "Merge when ready"} + + { + if (checked) { + if (isReady) { + enqueueMutation.mutate({ owner, repo, number, expectedHeadOid: headSha }); + } else { + autoMergeMutation.mutate({ owner, repo, number, enabled: true, mergeMethod }); + } + } else { + if (mergeInfo.isInMergeQueue && mergeInfo.entry) { + dequeueMutation.mutate({ + owner, + repo, + number, + mergeQueueEntryId: mergeInfo.entry.id, + }); + } + if (mergeInfo.autoMergeEnabled) { + autoMergeMutation.mutate({ owner, repo, number, enabled: false }); + } + } + }} + /> +
+ ); + } + + if (mergeInfo.autoMergeAllowed || mergeInfo.viewerCanDisableAutoMerge) { + return ( +
+ + {anyPending ? "Updating…" : "Merge when ready"} + + { + autoMergeMutation.mutate({ + owner, + repo, + number, + enabled: checked, + ...(checked && { mergeMethod }), + }); + }} + /> +
+ ); + } + + return ( +

+ {summary.isTransient + ? "Waiting for status checks to complete." + : "This pull request cannot be merged yet."} +

+ ); +} + +export interface MergeStatusProps { + mergeInfo: MergeStatusInfo; + owner: string; + repo: string; + number: number; + headSha: string; +} + +export function MergeStatus({ mergeInfo, owner, repo, number, headSha }: MergeStatusProps) { + const summary = getMergeStatusSummary(mergeInfo); + const MergeIcon = summary.icon; + + return ( + + + + + + + + + ); +} diff --git a/packages/web/src/components/pull-request/pull-request-header.tsx b/packages/web/src/components/pull-request/pull-request-header.tsx index cf26289..100b8aa 100644 --- a/packages/web/src/components/pull-request/pull-request-header.tsx +++ b/packages/web/src/components/pull-request/pull-request-header.tsx @@ -1,30 +1,43 @@ -import type { DeploymentLink } from "@stagereview/types/pull-request"; import { + type DeploymentLink, type GitHubPullRequest, MERGE_STATE_STATUS, MERGEABLE_STATE, type MergeStatusInfo, PULL_REQUEST_STATUS, } from "@stagereview/types/pull-request"; -import { GitBranch, Github, ScanSearch } from "lucide-react"; -import { useCallback } from "react"; +import { useMutation } from "@tanstack/react-query"; +import { Check, GitBranch, Github, Pencil, ScanSearch, X } from "lucide-react"; +import { useCallback, useRef, useState } from "react"; import { useHotkeys } from "react-hotkeys-hook"; import { CIChecks } from "@/components/pull-request/ci-checks"; -import { getMergeStatusSummary } from "@/components/pull-request/merge-status-summary"; +import { MergeStatus } from "@/components/pull-request/merge-status"; +import { PullRequestStatus } from "@/components/pull-request/pull-request-status"; import { Reviewers } from "@/components/pull-request/reviewers"; import { DeploymentLinkList } from "@/components/shared/deployment-link-list"; import { ShortcutTooltip } from "@/components/shared/shortcut-tooltip"; import { getUserDisplay, UserName } from "@/components/shared/user-name"; +import { + AlertDialog, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from "@/components/ui/alert-dialog"; import { Avatar, AvatarFallback, AvatarImage } from "@/components/ui/avatar"; import { Button } from "@/components/ui/button"; +import { Input } from "@/components/ui/input"; import { Popover, PopoverContent, PopoverTrigger } from "@/components/ui/popover"; import { toast } from "@/components/ui/sonner"; import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip"; import { formatTimeAgo } from "@/lib/format"; import { KEYBOARD_SHORTCUTS } from "@/lib/keyboard-shortcuts"; import { usePullRequestContext } from "@/lib/pull-request-context"; +import { titleMutationOptions, useInvalidatePullRequest } from "@/lib/pull-request-mutations"; import { usePullRequestChecks } from "@/lib/use-pull-request"; -import { cn } from "@/lib/utils"; +import { usePullRequestStatusActions } from "@/lib/use-pull-request-status-actions"; import { getPullRequestStatusInfo } from "@/lib/utils/pull-request-status"; function HeaderDeploymentPopover({ deploymentLinks }: { deploymentLinks: DeploymentLink[] }) { @@ -52,31 +65,16 @@ function HeaderDeploymentPopover({ deploymentLinks }: { deploymentLinks: Deploym ); } -function MergeStatusPill({ mergeInfo }: { mergeInfo: MergeStatusInfo }) { - const summary = getMergeStatusSummary(mergeInfo); - const Icon = summary.icon; - return ( - - - {summary.label} - - ); -} - export interface PullRequestHeaderProps { pullRequest: GitHubPullRequest; mergeInfo?: MergeStatusInfo; } export function PullRequestHeader({ pullRequest, mergeInfo }: PullRequestHeaderProps) { - const { runId } = usePullRequestContext(); - const status = getPullRequestStatusInfo(pullRequest); - const StatusIcon = status.icon; + const { runId, owner, repo } = usePullRequestContext(); + const inMergeQueue = mergeInfo?.isInMergeQueue; + const mergeQueuePosition = mergeInfo?.entry?.position; + const status = getPullRequestStatusInfo(pullRequest, { inMergeQueue, mergeQueuePosition }); const authorProfileUrl = pullRequest.user ? getUserDisplay(pullRequest.user).profileUrl : null; const isOpen = pullRequest.state === PULL_REQUEST_STATUS.OPEN && !pullRequest.merged_at && !pullRequest.draft; @@ -93,6 +91,47 @@ export function PullRequestHeader({ pullRequest, mergeInfo }: PullRequestHeaderP const deploymentLinks = checksData?.deploymentLinks ?? []; const hasChecks = checksData && checksData.items.length > 0; + // --- Title editing --- + const [isEditing, setIsEditing] = useState(false); + const [editValue, setEditValue] = useState(pullRequest.title); + const inputRef = useRef(null); + const invalidate = useInvalidatePullRequest(runId); + + const updateMutation = useMutation({ + ...titleMutationOptions(runId), + onSuccess: async () => { + await invalidate(); + setIsEditing(false); + toast.success("Title updated"); + }, + onError: () => { + setEditValue(pullRequest.title); + setIsEditing(false); + toast.error("Failed to update title"); + }, + }); + + function startEditing() { + setEditValue(pullRequest.title); + setIsEditing(true); + requestAnimationFrame(() => inputRef.current?.focus()); + } + + function cancelEditing() { + setIsEditing(false); + setEditValue(pullRequest.title); + } + + function submitEdit() { + const trimmed = editValue.trim(); + if (!trimmed) return; + if (trimmed === pullRequest.title) { + setIsEditing(false); + return; + } + updateMutation.mutate({ number: pullRequest.number, title: trimmed }); + } + const copyToClipboard = useCallback((text: string, label: string) => { navigator.clipboard.writeText(text).then( () => toast.success(`Copied ${label} to clipboard`), @@ -106,16 +145,21 @@ export function PullRequestHeader({ pullRequest, mergeInfo }: PullRequestHeaderP useHotkeys(KEYBOARD_SHORTCUTS.COPY_BRANCH_NAME.hotkey, copyBranchName); - const statusPill = ( -
- - {status.label} -
+ const statusActions = usePullRequestStatusActions({ runId, pullRequest }); + + const statusDropdown = ( + ); const externalLinks = ( @@ -157,82 +201,162 @@ export function PullRequestHeader({ pullRequest, mergeInfo }: PullRequestHeaderP ); return ( -
- {/* Row 1: Status + Title + External links */} -
-
- {statusPill} - {externalLinks} + <> +
+ {/* Row 1: Status + Title + External links */} +
+
+ {statusDropdown} + {externalLinks} +
+
+
{statusDropdown}
+ {isEditing ? ( + <> + setEditValue(e.target.value)} + onKeyDown={(e) => { + if (e.key === "Enter") submitEdit(); + if (e.key === "Escape") cancelEditing(); + }} + disabled={updateMutation.isPending} + className="min-w-0 flex-1 font-semibold text-xl @xl:text-2xl" + /> + + + + ) : ( + <> +

+ {pullRequest.title} + + #{pullRequest.number} + +

+ + + )} +
{externalLinks}
+
-
-
{statusPill}
-

- {pullRequest.title} - - #{pullRequest.number} - -

-
{externalLinks}
-
-
- {/* Row 2: Metadata */} -
- {pullRequest.user && authorProfileUrl && ( - <> - - - - - {pullRequest.user.login[0]?.toUpperCase()} - - - - - - {" opened "} - {formatTimeAgo(pullRequest.created_at)} - - - - )} -
+ + → + + + + + Copy base branch name + + {isOpenOrDraft && ( + <> + + {hasMergeData && mergeInfo && ( + + )} + {hasChecks && } + + )} + + + + + { + if (!statusActions.isClosePending) statusActions.setShowCloseDialog(open); + }} + > + + + Close pull request + + Are you sure you want to close this pull request? You can reopen it later. + + + + Cancel + + + + + ); } diff --git a/packages/web/src/components/pull-request/pull-request-status.tsx b/packages/web/src/components/pull-request/pull-request-status.tsx new file mode 100644 index 0000000..2214146 --- /dev/null +++ b/packages/web/src/components/pull-request/pull-request-status.tsx @@ -0,0 +1,152 @@ +import { type GitHubPullRequest, PULL_REQUEST_STATUS } from "@stagereview/types/pull-request"; +import { + ChevronDown, + GitPullRequest, + GitPullRequestClosed, + GitPullRequestDraft, +} from "lucide-react"; +import { useEffect, useRef, useState } from "react"; +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuItem, + DropdownMenuTrigger, +} from "@/components/ui/dropdown-menu"; +import { cn } from "@/lib/utils"; +import type { PullRequestStatusInfo } from "@/lib/utils/pull-request-status"; + +interface PullRequestStatusDropdownProps { + pullRequest: GitHubPullRequest; + statusInfo: PullRequestStatusInfo; + onClose: () => void; + onReopen: () => void; + onConvertToDraft: () => void; + onMarkReady: () => void; + isDraftTogglePending: boolean; + isClosePending: boolean; + isReopenPending: boolean; + inMergeQueue?: boolean; +} + +export function PullRequestStatus({ + pullRequest, + statusInfo, + onClose, + onReopen, + onConvertToDraft, + onMarkReady, + isDraftTogglePending, + isClosePending, + isReopenPending, + inMergeQueue, +}: PullRequestStatusDropdownProps) { + const [open, setOpen] = useState(false); + const hasPendingAction = useRef(false); + + const anyPending = isDraftTogglePending || isClosePending || isReopenPending; + + useEffect(() => { + if (anyPending) { + hasPendingAction.current = true; + } else if (hasPendingAction.current) { + hasPendingAction.current = false; + setOpen(false); + } + }, [anyPending]); + + const StatusIcon = statusInfo.icon; + const isMerged = !!pullRequest.merged_at; + const isOpen = pullRequest.state === PULL_REQUEST_STATUS.OPEN; + const isClosed = pullRequest.state === PULL_REQUEST_STATUS.CLOSED && !isMerged; + + const canToggleDraft = isOpen && !inMergeQueue; + + // Merged is a terminal state — static pill, no dropdown + if (isMerged) { + return ( +
+ + {statusInfo.label} +
+ ); + } + + return ( + + + + + + {isOpen && ( + <> + {canToggleDraft && + (pullRequest.draft ? ( + { + e.preventDefault(); + onMarkReady(); + }} + disabled={isDraftTogglePending} + > + + {isDraftTogglePending ? "Marking ready…" : "Mark as ready"} + + ) : ( + { + e.preventDefault(); + onConvertToDraft(); + }} + disabled={isDraftTogglePending} + > + + {isDraftTogglePending ? "Converting to draft…" : "Convert to draft"} + + ))} + { + e.preventDefault(); + onClose(); + }} + disabled={isClosePending} + > + + Close pull request + + + )} + {isClosed && ( + { + e.preventDefault(); + onReopen(); + }} + disabled={isReopenPending} + > + {isReopenPending ? "Reopening…" : "Reopen pull request"} + + )} + + + ); +} diff --git a/packages/web/src/components/pull-request/reviewers.tsx b/packages/web/src/components/pull-request/reviewers.tsx index 324fe15..7240c4c 100644 --- a/packages/web/src/components/pull-request/reviewers.tsx +++ b/packages/web/src/components/pull-request/reviewers.tsx @@ -2,7 +2,9 @@ import { REVIEWER_STATUS, type Reviewer, type ReviewerStatus, + type ReviewUser, } from "@stagereview/types/pull-request"; +import { useMutation } from "@tanstack/react-query"; import { Check, ChevronDown, @@ -10,6 +12,7 @@ import { Loader2, type LucideIcon, MessageSquare, + RefreshCw, Users, X, } from "lucide-react"; @@ -18,9 +21,17 @@ import { BotBadge } from "@/components/shared/bot-badge"; import { ReviewerAvatars } from "@/components/shared/reviewer-avatars"; import { getUserDisplay } from "@/components/shared/user-utils"; import { Avatar, AvatarFallback, AvatarImage } from "@/components/ui/avatar"; +import { Button } from "@/components/ui/button"; +import { Input } from "@/components/ui/input"; import { Popover, PopoverContent, PopoverTrigger } from "@/components/ui/popover"; +import { toast } from "@/components/ui/sonner"; import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip"; import { usePullRequestContext } from "@/lib/pull-request-context"; +import { + addReviewerMutationOptions, + removeReviewerMutationOptions, +} from "@/lib/pull-request-mutations"; +import { useReviewerManager } from "@/lib/use-reviewer-manager"; import { cn } from "@/lib/utils"; const STATUS_DESCRIPTIONS: Record = { @@ -46,10 +57,62 @@ function StatusIcon({ status }: { status: ReviewerStatus }) { return ; } -function ReviewerRow({ reviewer }: { reviewer: Reviewer }) { +interface ReviewerRowProps { + reviewer: Reviewer; + owner: string; + repo: string; + pullNumber: number; + onRemoveMutate: (login: string) => void; + onRemoveError: (login: string) => void; + invalidatePullRequestQueries: () => void; +} + +function ReviewerRow({ + reviewer, + owner, + repo, + pullNumber, + onRemoveMutate, + onRemoveError, + invalidatePullRequestQueries, +}: ReviewerRowProps) { + const { runId } = usePullRequestContext(); + const removeMutation = useMutation({ + ...removeReviewerMutationOptions(runId), + onMutate: () => onRemoveMutate(reviewer.user.login), + onSuccess: () => { + invalidatePullRequestQueries(); + toast.success("Reviewer removed"); + }, + onError: () => { + onRemoveError(reviewer.user.login); + toast.error("Failed to remove reviewer"); + }, + }); + + const rerequestMutation = useMutation({ + ...addReviewerMutationOptions(runId), + onSuccess: () => { + invalidatePullRequestQueries(); + toast.success("Review re-requested"); + }, + onError: () => { + toast.error("Failed to re-request review"); + }, + }); + const { isBot, displayName, profileUrl } = getUserDisplay(reviewer.user); + const isRequested = + reviewer.status === REVIEWER_STATUS.REQUESTED || reviewer.status === REVIEWER_STATUS.PENDING; + const hasReviewed = + reviewer.status === REVIEWER_STATUS.APPROVED || + reviewer.status === REVIEWER_STATUS.CHANGES_REQUESTED || + reviewer.status === REVIEWER_STATUS.COMMENTED || + reviewer.status === REVIEWER_STATUS.DISMISSED; + const isPending = removeMutation.isPending || rerequestMutation.isPending; + return ( -
+ ); } +interface CollaboratorRowProps { + user: ReviewUser; + owner: string; + repo: string; + pullNumber: number; + onSuccess: () => void; + onAddMutate: (user: ReviewUser) => void; + onAddError: (login: string) => void; + invalidatePullRequestQueries: () => void; +} + +function CollaboratorRow({ + user, + owner, + repo, + pullNumber, + onSuccess, + onAddMutate, + onAddError, + invalidatePullRequestQueries, +}: CollaboratorRowProps) { + const { runId } = usePullRequestContext(); + const addMutation = useMutation({ + ...addReviewerMutationOptions(runId), + onMutate: () => onAddMutate(user), + onSuccess: () => { + onSuccess(); + invalidatePullRequestQueries(); + toast.success("Reviewer requested"); + }, + onError: () => { + onAddError(user.login); + toast.error("Failed to add reviewer"); + }, + }); + + return ( + + ); +} + export function Reviewers() { const [open, setOpen] = useState(false); - const { reviews } = usePullRequestContext(); - const reviewers = reviews?.reviewers ?? []; + const [search, setSearch] = useState(""); + + const { + owner, + repo, + pullNumber, + reviews, + reviewers, + collaborators, + filteredCollaborators, + onAddMutate, + onAddError, + onRemoveMutate, + onRemoveError, + invalidatePullRequestQueries, + } = useReviewerManager({ open, search }); return ( - + { + setOpen(newOpen); + if (!newOpen) setSearch(""); + }} + >
- {reviewers.length > 0 ? ( + + {reviewers.length > 0 && (
{reviewers.map((reviewer) => ( - + ))}
- ) : ( -

No reviewers yet

)} + +
+ setSearch(e.target.value)} + className="h-8 text-sm" + /> +
+ {filteredCollaborators.length > 0 ? ( + filteredCollaborators.map((user) => ( + setSearch("")} + onAddMutate={onAddMutate} + onAddError={onAddError} + invalidatePullRequestQueries={invalidatePullRequestQueries} + /> + )) + ) : !collaborators ? ( +
+ +
+ ) : search ? ( +

+ No matching collaborators +

+ ) : null} +
+
diff --git a/packages/web/src/components/ui/alert-dialog.tsx b/packages/web/src/components/ui/alert-dialog.tsx new file mode 100644 index 0000000..192ea7c --- /dev/null +++ b/packages/web/src/components/ui/alert-dialog.tsx @@ -0,0 +1,132 @@ +import * as AlertDialogPrimitive from "@radix-ui/react-alert-dialog"; +import type * as React from "react"; +import { buttonVariants } from "@/components/ui/button"; +import { cn } from "@/lib/utils"; + +function AlertDialog({ ...props }: React.ComponentProps) { + return ; +} + +function AlertDialogTrigger({ + ...props +}: React.ComponentProps) { + return ; +} + +function AlertDialogPortal({ ...props }: React.ComponentProps) { + return ; +} + +function AlertDialogOverlay({ + className, + ...props +}: React.ComponentProps) { + return ( + + ); +} + +function AlertDialogContent({ + className, + ...props +}: React.ComponentProps) { + return ( + + + + + ); +} + +function AlertDialogHeader({ className, ...props }: React.ComponentProps<"div">) { + return ( +
+ ); +} + +function AlertDialogFooter({ className, ...props }: React.ComponentProps<"div">) { + return ( +
+ ); +} + +function AlertDialogTitle({ + className, + ...props +}: React.ComponentProps) { + return ( + + ); +} + +function AlertDialogDescription({ + className, + ...props +}: React.ComponentProps) { + return ( + + ); +} + +function AlertDialogAction({ + className, + ...props +}: React.ComponentProps) { + return ; +} + +function AlertDialogCancel({ + className, + ...props +}: React.ComponentProps) { + return ( + + ); +} + +export { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogOverlay, + AlertDialogPortal, + AlertDialogTitle, + AlertDialogTrigger, +}; diff --git a/packages/web/src/lib/pull-request-mutations.ts b/packages/web/src/lib/pull-request-mutations.ts new file mode 100644 index 0000000..5015da9 --- /dev/null +++ b/packages/web/src/lib/pull-request-mutations.ts @@ -0,0 +1,126 @@ +import type { PullRequestMergeMethod } from "@stagereview/types/pull-request"; +import { useQueryClient } from "@tanstack/react-query"; +import { jsonFetch } from "@/lib/use-view-state"; + +function prPath(runId: string, suffix: string): string { + return `/api/runs/${encodeURIComponent(runId)}/pull-request${suffix}`; +} + +function write( + runId: string, + suffix: string, + method: "POST" | "PATCH" | "DELETE", + body: Record, +): Promise { + return jsonFetch(prPath(runId, suffix), { + method, + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }); +} + +/** Invalidate every PR-derived query for a run after a mutation. */ +export function useInvalidatePullRequest(runId: string): () => Promise { + const queryClient = useQueryClient(); + return () => + Promise.all([ + queryClient.invalidateQueries({ queryKey: ["pull-request", runId] }), + queryClient.invalidateQueries({ queryKey: ["pull-request-reviews", runId] }), + queryClient.invalidateQueries({ queryKey: ["pull-request-merge-status", runId] }), + queryClient.invalidateQueries({ queryKey: ["pull-request-checks", runId] }), + ]); +} + +// Mutation-option factories — mirror hosted's `orpc.pullRequests.X.mutationOptions()` +// so the vendored components keep their `useMutation({ ...factory, onSuccess })` shape. +// They accept the components' `{ owner, repo, number, ... }` call shape (owner/repo +// are ignored — the server resolves the repo from the run). + +/** Vendored components call `.mutate({ owner, repo, ... })`; accept and ignore those. */ +type RepoVars = { owner?: string; repo?: string }; + +export function titleMutationOptions(runId: string) { + return { + mutationFn: (v: { number: number; title: string }) => + write(runId, "/title", "PATCH", { number: v.number, title: v.title }), + }; +} + +export function closeMutationOptions(runId: string) { + return { + mutationFn: (v: { number: number }) => write(runId, "/close", "POST", { number: v.number }), + }; +} + +export function reopenMutationOptions(runId: string) { + return { + mutationFn: (v: { number: number }) => write(runId, "/reopen", "POST", { number: v.number }), + }; +} + +export function draftMutationOptions(runId: string) { + return { + mutationFn: (v: { number: number; draft: boolean }) => + write(runId, "/draft", "POST", { number: v.number, draft: v.draft }), + }; +} + +export function mergeMutationOptions(runId: string) { + return { + mutationFn: ( + v: RepoVars & { + number: number; + mergeMethod: PullRequestMergeMethod; + expectedHeadOid?: string; + }, + ) => + write(runId, "/merge", "POST", { + number: v.number, + mergeMethod: v.mergeMethod, + expectedHeadOid: v.expectedHeadOid, + }), + }; +} + +// Merge-queue enqueue maps to "enable auto-merge" — gh enqueues when ready. +export function enqueueMutationOptions(runId: string) { + return { + mutationFn: (v: RepoVars & { number: number; expectedHeadOid?: string }) => + write(runId, "/auto-merge", "POST", { number: v.number, enabled: true }), + }; +} + +export function setAutoMergeMutationOptions(runId: string) { + return { + mutationFn: ( + v: RepoVars & { number: number; enabled: boolean; mergeMethod?: PullRequestMergeMethod }, + ) => + write(runId, "/auto-merge", "POST", { + number: v.number, + enabled: v.enabled, + mergeMethod: v.mergeMethod, + }), + }; +} + +// Dequeue maps to "disable auto-merge". +export function dequeueMutationOptions(runId: string) { + return { + mutationFn: (v: RepoVars & { number: number; mergeQueueEntryId: string }) => + write(runId, "/auto-merge", "POST", { number: v.number, enabled: false }), + }; +} + +export function addReviewerMutationOptions(runId: string) { + return { + mutationFn: (v: RepoVars & { number: number; reviewers: string[] }) => + write(runId, "/reviewers", "POST", { number: v.number, reviewers: v.reviewers }), + }; +} + +export function removeReviewerMutationOptions(runId: string) { + return { + mutationFn: (v: RepoVars & { number: number; reviewer: string }) => + write(runId, "/reviewers", "DELETE", { number: v.number, reviewers: [v.reviewer] }), + }; +} diff --git a/packages/web/src/lib/use-pull-request-status-actions.ts b/packages/web/src/lib/use-pull-request-status-actions.ts new file mode 100644 index 0000000..60ec699 --- /dev/null +++ b/packages/web/src/lib/use-pull-request-status-actions.ts @@ -0,0 +1,68 @@ +import type { GitHubPullRequest } from "@stagereview/types/pull-request"; +import { useMutation } from "@tanstack/react-query"; +import { useState } from "react"; +import { toast } from "@/components/ui/sonner"; +import { + closeMutationOptions, + draftMutationOptions, + reopenMutationOptions, + useInvalidatePullRequest, +} from "@/lib/pull-request-mutations"; + +interface Options { + runId: string; + pullRequest: GitHubPullRequest; +} + +export function usePullRequestStatusActions({ runId, pullRequest }: Options) { + const invalidate = useInvalidatePullRequest(runId); + const [showCloseDialog, setShowCloseDialog] = useState(false); + + const draftMutation = useMutation({ + ...draftMutationOptions(runId), + onSuccess: async () => { + await invalidate(); + toast.success(pullRequest.draft ? "Marked as ready for review" : "Converted to draft"); + }, + onError: () => toast.error("Failed to update draft status"), + }); + + const closeMutation = useMutation({ + ...closeMutationOptions(runId), + onSuccess: async () => { + await invalidate(); + setShowCloseDialog(false); + toast.success("Pull request closed"); + }, + onError: () => { + setShowCloseDialog(false); + toast.error("Failed to close pull request"); + }, + }); + + const reopenMutation = useMutation({ + ...reopenMutationOptions(runId), + onSuccess: async () => { + await invalidate(); + toast.success("Pull request reopened"); + }, + onError: () => toast.error("Failed to reopen pull request"), + }); + + const toggleDraft = () => { + draftMutation.mutate({ number: pullRequest.number, draft: !pullRequest.draft }); + }; + + return { + onClose: () => setShowCloseDialog(true), + onReopen: () => reopenMutation.mutate({ number: pullRequest.number }), + onConvertToDraft: toggleDraft, + onMarkReady: toggleDraft, + isDraftTogglePending: draftMutation.isPending, + isClosePending: closeMutation.isPending, + isReopenPending: reopenMutation.isPending, + showCloseDialog, + setShowCloseDialog, + confirmClose: () => closeMutation.mutate({ number: pullRequest.number }), + }; +} diff --git a/packages/web/src/lib/use-pull-request.ts b/packages/web/src/lib/use-pull-request.ts index 4d4b299..5669713 100644 --- a/packages/web/src/lib/use-pull-request.ts +++ b/packages/web/src/lib/use-pull-request.ts @@ -1,6 +1,8 @@ import { type ChecksResponse, ChecksResponseSchema, + type CollaboratorsResponse, + CollaboratorsResponseSchema, type MergeStatusResponse, MergeStatusResponseSchema, type PullRequestResponse, @@ -75,3 +77,14 @@ export function usePullRequestMergeStatus(runId: string, number: number | null, ...LIVE, }); } + +export function usePullRequestCollaborators(runId: string, enabled: boolean) { + return useQuery({ + queryKey: ["pull-request-collaborators", runId], + queryFn: !enabled + ? skipToken + : async () => + CollaboratorsResponseSchema.parse(await jsonFetch(prPath(runId, "/collaborators"))), + staleTime: 5 * 60 * 1000, + }); +} diff --git a/packages/web/src/lib/use-reviewer-manager.ts b/packages/web/src/lib/use-reviewer-manager.ts new file mode 100644 index 0000000..71a65ff --- /dev/null +++ b/packages/web/src/lib/use-reviewer-manager.ts @@ -0,0 +1,126 @@ +import { REVIEWER_STATUS, type Reviewer, type ReviewUser } from "@stagereview/types/pull-request"; +import { useCallback, useEffect, useMemo, useState } from "react"; +import { filterAndSortReviewers } from "@/components/shared/reviewer-avatars"; +import { usePullRequestContext } from "@/lib/pull-request-context"; +import { useInvalidatePullRequest } from "@/lib/pull-request-mutations"; +import { usePullRequestCollaborators } from "@/lib/use-pull-request"; + +interface UseReviewerManagerOptions { + open: boolean; + search: string; +} + +export function useReviewerManager({ open, search }: UseReviewerManagerOptions) { + const { runId, owner, repo, number, pullRequest, reviews } = usePullRequestContext(); + const invalidatePullRequestQueries = useInvalidatePullRequest(runId); + const [optimisticAdditions, setOptimisticAdditions] = useState>( + () => new Map(), + ); + const [optimisticRemovals, setOptimisticRemovals] = useState>(() => new Set()); + + const { data: collaboratorsData } = usePullRequestCollaborators(runId, open); + const collaborators = collaboratorsData?.collaborators ?? null; + + const serverReviewers = useMemo(() => { + if (!reviews?.reviewers) return []; + return filterAndSortReviewers(reviews.reviewers, pullRequest.user?.login); + }, [reviews?.reviewers, pullRequest.user?.login]); + + // Clear optimistic state once server data reflects the changes. + useEffect(() => { + const serverLogins = new Set(serverReviewers.map((r) => r.user.login)); + setOptimisticAdditions((prev) => { + const next = new Map([...prev].filter(([login]) => !serverLogins.has(login))); + return next.size === prev.size ? prev : next; + }); + setOptimisticRemovals((prev) => { + const next = new Set( + [...prev].filter((login) => serverLogins.has(login) || optimisticAdditions.has(login)), + ); + return next.size === prev.size ? prev : next; + }); + }, [serverReviewers, optimisticAdditions]); + + const reviewers = useMemo(() => { + const filtered = serverReviewers.filter((r) => !optimisticRemovals.has(r.user.login)); + const additions = [...optimisticAdditions.values()].filter( + (r) => !optimisticRemovals.has(r.user.login), + ); + return filterAndSortReviewers([...filtered, ...additions], pullRequest.user?.login); + }, [serverReviewers, optimisticRemovals, optimisticAdditions, pullRequest.user?.login]); + + const currentReviewerLogins = useMemo( + () => new Set(reviewers.map((r) => r.user.login)), + [reviewers], + ); + + const availableCollaborators = useMemo(() => { + if (!collaborators) return []; + const authorLogin = pullRequest.user?.login; + return collaborators.filter( + (c) => c.type !== "Bot" && c.login !== authorLogin && !currentReviewerLogins.has(c.login), + ); + }, [collaborators, pullRequest.user?.login, currentReviewerLogins]); + + const filteredCollaborators = useMemo(() => { + if (!search) return availableCollaborators; + const queryText = search.toLowerCase(); + return availableCollaborators.filter((c) => c.login.toLowerCase().includes(queryText)); + }, [availableCollaborators, search]); + + const onAddMutate = useCallback((user: ReviewUser) => { + setOptimisticRemovals((prev) => { + if (!prev.has(user.login)) return prev; + const next = new Set(prev); + next.delete(user.login); + return next; + }); + setOptimisticAdditions((prev) => { + const next = new Map(prev); + next.set(user.login, { user, status: REVIEWER_STATUS.REQUESTED }); + return next; + }); + }, []); + + const onAddError = useCallback((login: string) => { + setOptimisticAdditions((prev) => { + const next = new Map(prev); + next.delete(login); + return next.size === prev.size ? prev : next; + }); + }, []); + + const onRemoveMutate = useCallback((login: string) => { + setOptimisticAdditions((prev) => { + if (!prev.has(login)) return prev; + const next = new Map(prev); + next.delete(login); + return next; + }); + setOptimisticRemovals((prev) => new Set(prev).add(login)); + }, []); + + const onRemoveError = useCallback((login: string) => { + setOptimisticRemovals((prev) => { + if (!prev.has(login)) return prev; + const next = new Set(prev); + next.delete(login); + return next; + }); + }, []); + + return { + owner, + repo, + pullNumber: number, + reviews, + reviewers, + collaborators, + filteredCollaborators, + onAddMutate, + onAddError, + onRemoveMutate, + onRemoveError, + invalidatePullRequestQueries, + }; +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 684aa88..4e00d87 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -94,6 +94,9 @@ importers: '@pierre/diffs': specifier: ^1.0.11 version: 1.1.20(react-dom@19.2.5(react@19.2.5))(react@19.2.5) + '@radix-ui/react-alert-dialog': + specifier: ^1.1.15 + version: 1.1.15(@types/react-dom@19.2.3(@types/react@19.2.14))(@types/react@19.2.14)(react-dom@19.2.5(react@19.2.5))(react@19.2.5) '@radix-ui/react-avatar': specifier: ^1.1.10 version: 1.1.10(@types/react-dom@19.2.3(@types/react@19.2.14))(@types/react@19.2.14)(react-dom@19.2.5(react@19.2.5))(react@19.2.5) From 30ee34823b389190b1cd2dd55b226124af5b3be7 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 1 Jun 2026 14:29:11 -0700 Subject: [PATCH 03/12] refactor(cli): move gh write adapters into the github module Relocate github-mutations.ts to github/mutations.ts and export it from the github barrel, so reads and writes live in one cohesive github/ module. Routes import the write adapters via github/index.js. No behavior change. --- packages/cli/src/github/index.ts | 11 +++++++++++ .../src/{github-mutations.ts => github/mutations.ts} | 6 +++--- packages/cli/src/routes/pull-request-mutations.ts | 2 +- 3 files changed, 15 insertions(+), 4 deletions(-) rename packages/cli/src/{github-mutations.ts => github/mutations.ts} (95%) diff --git a/packages/cli/src/github/index.ts b/packages/cli/src/github/index.ts index 638250f..c065068 100644 --- a/packages/cli/src/github/index.ts +++ b/packages/cli/src/github/index.ts @@ -1,2 +1,13 @@ +export { + addReviewers, + closePullRequest, + editTitle, + listCollaborators, + mergePullRequest, + removeReviewers, + reopenPullRequest, + setAutoMerge, + setDraft, +} from "./mutations.js"; export { getChecks, getMergeStatus, getPullRequest, getReviews } from "./pull-request.js"; export { type GitHubRepo, isGitHubRemote, parseGitHubRepo } from "./repo.js"; diff --git a/packages/cli/src/github-mutations.ts b/packages/cli/src/github/mutations.ts similarity index 95% rename from packages/cli/src/github-mutations.ts rename to packages/cli/src/github/mutations.ts index 6862a23..4470acc 100644 --- a/packages/cli/src/github-mutations.ts +++ b/packages/cli/src/github/mutations.ts @@ -4,14 +4,14 @@ import { PULL_REQUEST_MERGE_METHOD, type PullRequestMergeMethod, } from "@stagereview/types/pull-request"; -import type { GitHubRepo } from "./github/index.js"; +import type { GitHubRepo } from "./repo.js"; const execFileAsync = promisify(execFile); /** * Run a `gh` write command in `repoRoot`. Unlike the read adapters in - * github.ts (which swallow errors to null), writes surface failures so the UI - * can toast them — the user explicitly asked to mutate their PR. + * pull-request.ts (which swallow errors to null), writes surface failures so + * the UI can toast them — the user explicitly asked to mutate their PR. */ async function ghWrite(args: string[], repoRoot: string): Promise { try { diff --git a/packages/cli/src/routes/pull-request-mutations.ts b/packages/cli/src/routes/pull-request-mutations.ts index 35990d7..be27336 100644 --- a/packages/cli/src/routes/pull-request-mutations.ts +++ b/packages/cli/src/routes/pull-request-mutations.ts @@ -14,7 +14,7 @@ import { reopenPullRequest, setAutoMerge, setDraft, -} from "../github-mutations.js"; +} from "../github/index.js"; import type { Route, RouteHandler } from "../server.js"; import { readJsonBody, writeJson } from "./json.js"; import { requireRepo, resolveRun } from "./pull-request-shared.js"; From 0bfac48a07ec79208ce6fc1c828e548f4602f816 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 1 Jun 2026 14:48:48 -0700 Subject: [PATCH 04/12] fix(pr-header): surface gh errors, forward head SHA, dedupe disable-auto MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR review feedback on the write layer: - write() now reads the server's { error } body on failure so toasts show the actionable gh stderr instead of a generic "POST … failed: 500". - Forward expectedHeadOid through the enqueue/auto-merge path into gh pr merge --auto --match-head-commit, guarding auto-merge against a stale head (parity with the direct-merge path). - The merge-queue off-toggle no longer fires both dequeue and disable-auto (which map to the same gh --disable-auto), avoiding a duplicate request. --- .../pull-request-mutations.routes.test.ts | 12 ++++++++ packages/cli/src/github/mutations.ts | 3 ++ .../cli/src/routes/pull-request-mutations.ts | 9 +++++- .../components/pull-request/merge-status.tsx | 24 +++++++-------- .../web/src/lib/pull-request-mutations.ts | 30 ++++++++++++++++--- 5 files changed, 61 insertions(+), 17 deletions(-) diff --git a/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts b/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts index cb2f082..af71b30 100644 --- a/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts +++ b/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts @@ -155,6 +155,18 @@ describe("pull-request mutation API", () => { expect(await ghArgs()).toEqual(["pr merge 7 --auto --merge", "pr merge 7 --disable-auto"]); }); + it("forwards expectedHeadOid to gh as --match-head-commit when enabling auto-merge", async () => { + const runId = insertRun(); + const res = await send(await start(), "POST", `/api/runs/${runId}/pull-request/auto-merge`, { + number: 7, + enabled: true, + mergeMethod: "SQUASH", + expectedHeadOid: SHA, + }); + expect(res.status).toBe(200); + expect(await ghArgs()).toEqual([`pr merge 7 --auto --squash --match-head-commit ${SHA}`]); + }); + it("adds and removes reviewers via gh pr edit", async () => { const runId = insertRun(); const port = await start(); diff --git a/packages/cli/src/github/mutations.ts b/packages/cli/src/github/mutations.ts index 4470acc..31fb6dd 100644 --- a/packages/cli/src/github/mutations.ts +++ b/packages/cli/src/github/mutations.ts @@ -70,10 +70,13 @@ export function setAutoMerge( number: number, enabled: boolean, mergeMethod?: PullRequestMergeMethod, + expectedHeadOid?: string, ): Promise { if (!enabled) return ghWrite(["pr", "merge", String(number), "--disable-auto"], repoRoot); const args = ["pr", "merge", String(number), "--auto"]; if (mergeMethod) args.push(MERGE_METHOD_FLAG[mergeMethod]); + // Guard against enabling auto-merge for a stale head the user hasn't seen. + if (expectedHeadOid) args.push("--match-head-commit", expectedHeadOid); return ghWrite(args, repoRoot); } diff --git a/packages/cli/src/routes/pull-request-mutations.ts b/packages/cli/src/routes/pull-request-mutations.ts index be27336..fa1dd26 100644 --- a/packages/cli/src/routes/pull-request-mutations.ts +++ b/packages/cli/src/routes/pull-request-mutations.ts @@ -37,6 +37,7 @@ const autoMergeInput = z.object({ number: numberField, enabled: z.boolean(), mergeMethod: mergeMethod.optional(), + expectedHeadOid: z.string().optional(), }); const reviewersInput = z.object({ number: numberField, reviewers: z.array(z.string()).min(1) }); @@ -127,7 +128,13 @@ export function pullRequestMutationRoutes(db: StageDb): Route[] { const input = await parseBody(req, res, autoMergeInput); if (!input) return; await runMutation(res, () => - setAutoMerge(run.repoRoot, input.number, input.enabled, input.mergeMethod), + setAutoMerge( + run.repoRoot, + input.number, + input.enabled, + input.mergeMethod, + input.expectedHeadOid, + ), ); }, }, diff --git a/packages/web/src/components/pull-request/merge-status.tsx b/packages/web/src/components/pull-request/merge-status.tsx index c08fad3..f6c5b23 100644 --- a/packages/web/src/components/pull-request/merge-status.tsx +++ b/packages/web/src/components/pull-request/merge-status.tsx @@ -174,18 +174,18 @@ function MergeActions({ } else { autoMergeMutation.mutate({ owner, repo, number, enabled: true, mergeMethod }); } - } else { - if (mergeInfo.isInMergeQueue && mergeInfo.entry) { - dequeueMutation.mutate({ - owner, - repo, - number, - mergeQueueEntryId: mergeInfo.entry.id, - }); - } - if (mergeInfo.autoMergeEnabled) { - autoMergeMutation.mutate({ owner, repo, number, enabled: false }); - } + } else if (mergeInfo.isInMergeQueue && mergeInfo.entry) { + dequeueMutation.mutate({ + owner, + repo, + number, + mergeQueueEntryId: mergeInfo.entry.id, + }); + } else if (mergeInfo.autoMergeEnabled) { + // `else if`: both dequeue and disabling auto-merge map to the same + // `gh pr merge --disable-auto` here, so fire only one to avoid a + // duplicate request (and a spurious error toast on the second). + autoMergeMutation.mutate({ owner, repo, number, enabled: false }); } }} /> diff --git a/packages/web/src/lib/pull-request-mutations.ts b/packages/web/src/lib/pull-request-mutations.ts index 5015da9..04cc9f1 100644 --- a/packages/web/src/lib/pull-request-mutations.ts +++ b/packages/web/src/lib/pull-request-mutations.ts @@ -1,22 +1,39 @@ import type { PullRequestMergeMethod } from "@stagereview/types/pull-request"; import { useQueryClient } from "@tanstack/react-query"; -import { jsonFetch } from "@/lib/use-view-state"; function prPath(runId: string, suffix: string): string { return `/api/runs/${encodeURIComponent(runId)}/pull-request${suffix}`; } -function write( +async function write( runId: string, suffix: string, method: "POST" | "PATCH" | "DELETE", body: Record, ): Promise { - return jsonFetch(prPath(runId, suffix), { + const path = prPath(runId, suffix); + const res = await fetch(path, { method, headers: { "Content-Type": "application/json" }, body: JSON.stringify(body), }); + const text = await res.text(); + if (!res.ok) { + // The server returns `{ error: }` on failure — surface it so the + // toast shows the actionable gh message, not a generic "POST … failed: 500". + let message = `${method} ${path} failed: ${res.status}`; + try { + const parsed: unknown = text ? JSON.parse(text) : null; + if (parsed && typeof parsed === "object" && "error" in parsed) { + const { error } = parsed as { error: unknown }; + if (typeof error === "string" && error) message = error; + } + } catch { + // non-JSON body — keep the generic message + } + throw new Error(message); + } + return text ? JSON.parse(text) : {}; } /** Invalidate every PR-derived query for a run after a mutation. */ @@ -83,10 +100,15 @@ export function mergeMutationOptions(runId: string) { } // Merge-queue enqueue maps to "enable auto-merge" — gh enqueues when ready. +// Forward the head SHA so the server can guard against a stale head (--match-head-commit). export function enqueueMutationOptions(runId: string) { return { mutationFn: (v: RepoVars & { number: number; expectedHeadOid?: string }) => - write(runId, "/auto-merge", "POST", { number: v.number, enabled: true }), + write(runId, "/auto-merge", "POST", { + number: v.number, + enabled: true, + expectedHeadOid: v.expectedHeadOid, + }), }; } From 2f7ce98a144cfb8346bf24edd6297a4052ee258f Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 1 Jun 2026 15:07:42 -0700 Subject: [PATCH 05/12] fix(web): surface gh error message on status-action failures Draft/close/reopen onError handlers now toast error.message when present (matching the merge actions), so the actionable gh stderr preserved by write() reaches the user instead of a generic failure toast. --- .../web/src/lib/use-pull-request-status-actions.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/web/src/lib/use-pull-request-status-actions.ts b/packages/web/src/lib/use-pull-request-status-actions.ts index 60ec699..9f28199 100644 --- a/packages/web/src/lib/use-pull-request-status-actions.ts +++ b/packages/web/src/lib/use-pull-request-status-actions.ts @@ -24,7 +24,8 @@ export function usePullRequestStatusActions({ runId, pullRequest }: Options) { await invalidate(); toast.success(pullRequest.draft ? "Marked as ready for review" : "Converted to draft"); }, - onError: () => toast.error("Failed to update draft status"), + onError: (error) => + toast.error(error instanceof Error ? error.message : "Failed to update draft status"), }); const closeMutation = useMutation({ @@ -34,9 +35,9 @@ export function usePullRequestStatusActions({ runId, pullRequest }: Options) { setShowCloseDialog(false); toast.success("Pull request closed"); }, - onError: () => { + onError: (error) => { setShowCloseDialog(false); - toast.error("Failed to close pull request"); + toast.error(error instanceof Error ? error.message : "Failed to close pull request"); }, }); @@ -46,7 +47,8 @@ export function usePullRequestStatusActions({ runId, pullRequest }: Options) { await invalidate(); toast.success("Pull request reopened"); }, - onError: () => toast.error("Failed to reopen pull request"), + onError: (error) => + toast.error(error instanceof Error ? error.message : "Failed to reopen pull request"), }); const toggleDraft = () => { From 1b783085dba70c90e6a9b23541bcd753b70dc050 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 1 Jun 2026 15:16:50 -0700 Subject: [PATCH 06/12] fix(cli): reject cross-origin PR mutations (CSRF guard) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mutation routes execute gh write commands and bind to a predictable loopback port, so a browser on any site could fire a no-cors POST to close/merge/retitle a PR while the CLI runs. Each mutation handler now requires a loopback Origin (browsers always send an accurate, unforgeable Origin cross-origin; non-browser clients like curl send none and are allowed). The read routes are unaffected — cross-origin reads get opaque responses. --- .../pull-request-mutations.routes.test.ts | 34 +++++++++++++++++++ .../cli/src/routes/pull-request-mutations.ts | 10 +++++- .../cli/src/routes/pull-request-shared.ts | 25 ++++++++++++++ 3 files changed, 68 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts b/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts index af71b30..b7ac9bc 100644 --- a/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts +++ b/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts @@ -82,6 +82,7 @@ function send( method: string, p: string, body: unknown, + extraHeaders: Record = {}, ): Promise<{ status: number; body: string }> { return new Promise((resolve, reject) => { const payload = JSON.stringify(body); @@ -95,6 +96,7 @@ function send( headers: { "Content-Type": "application/json", "Content-Length": Buffer.byteLength(payload), + ...extraHeaders, }, }, (res) => { @@ -184,6 +186,38 @@ describe("pull-request mutation API", () => { ]); }); + it("rejects a cross-origin mutation (CSRF guard) without invoking gh", async () => { + const runId = insertRun(); + const port = await start(); + const res = await send( + port, + "POST", + `/api/runs/${runId}/pull-request/close`, + { number: 7 }, + { + Origin: "https://evil.example", + }, + ); + expect(res.status).toBe(403); + expect(await ghArgs()).toEqual([]); + }); + + it("allows a mutation from a loopback origin", async () => { + const runId = insertRun(); + const port = await start(); + const res = await send( + port, + "POST", + `/api/runs/${runId}/pull-request/close`, + { number: 7 }, + { + Origin: `http://127.0.0.1:${port}`, + }, + ); + expect(res.status).toBe(200); + expect(await ghArgs()).toEqual(["pr close 7"]); + }); + it("rejects an invalid body", async () => { const runId = insertRun(); const res = await send(await start(), "POST", `/api/runs/${runId}/pull-request/draft`, { diff --git a/packages/cli/src/routes/pull-request-mutations.ts b/packages/cli/src/routes/pull-request-mutations.ts index fa1dd26..2a4d8b9 100644 --- a/packages/cli/src/routes/pull-request-mutations.ts +++ b/packages/cli/src/routes/pull-request-mutations.ts @@ -17,7 +17,7 @@ import { } from "../github/index.js"; import type { Route, RouteHandler } from "../server.js"; import { readJsonBody, writeJson } from "./json.js"; -import { requireRepo, resolveRun } from "./pull-request-shared.js"; +import { enforceSameOrigin, requireRepo, resolveRun } from "./pull-request-shared.js"; type Req = Parameters[0]; type Res = Parameters[1]; @@ -66,6 +66,7 @@ export function pullRequestMutationRoutes(db: StageDb): Route[] { method: "PATCH", pattern: "/api/runs/:runId/pull-request/title", handler: async (req, res, params) => { + if (!enforceSameOrigin(req, res)) return; const run = resolveRun(db, params, res); if (!run) return; const input = await parseBody(req, res, titleInput); @@ -77,6 +78,7 @@ export function pullRequestMutationRoutes(db: StageDb): Route[] { method: "POST", pattern: "/api/runs/:runId/pull-request/close", handler: async (req, res, params) => { + if (!enforceSameOrigin(req, res)) return; const run = resolveRun(db, params, res); if (!run) return; const input = await parseBody(req, res, numberInput); @@ -88,6 +90,7 @@ export function pullRequestMutationRoutes(db: StageDb): Route[] { method: "POST", pattern: "/api/runs/:runId/pull-request/reopen", handler: async (req, res, params) => { + if (!enforceSameOrigin(req, res)) return; const run = resolveRun(db, params, res); if (!run) return; const input = await parseBody(req, res, numberInput); @@ -99,6 +102,7 @@ export function pullRequestMutationRoutes(db: StageDb): Route[] { method: "POST", pattern: "/api/runs/:runId/pull-request/draft", handler: async (req, res, params) => { + if (!enforceSameOrigin(req, res)) return; const run = resolveRun(db, params, res); if (!run) return; const input = await parseBody(req, res, draftInput); @@ -110,6 +114,7 @@ export function pullRequestMutationRoutes(db: StageDb): Route[] { method: "POST", pattern: "/api/runs/:runId/pull-request/merge", handler: async (req, res, params) => { + if (!enforceSameOrigin(req, res)) return; const run = resolveRun(db, params, res); if (!run) return; const input = await parseBody(req, res, mergeInput); @@ -123,6 +128,7 @@ export function pullRequestMutationRoutes(db: StageDb): Route[] { method: "POST", pattern: "/api/runs/:runId/pull-request/auto-merge", handler: async (req, res, params) => { + if (!enforceSameOrigin(req, res)) return; const run = resolveRun(db, params, res); if (!run) return; const input = await parseBody(req, res, autoMergeInput); @@ -142,6 +148,7 @@ export function pullRequestMutationRoutes(db: StageDb): Route[] { method: "POST", pattern: "/api/runs/:runId/pull-request/reviewers", handler: async (req, res, params) => { + if (!enforceSameOrigin(req, res)) return; const run = resolveRun(db, params, res); if (!run) return; const input = await parseBody(req, res, reviewersInput); @@ -153,6 +160,7 @@ export function pullRequestMutationRoutes(db: StageDb): Route[] { method: "DELETE", pattern: "/api/runs/:runId/pull-request/reviewers", handler: async (req, res, params) => { + if (!enforceSameOrigin(req, res)) return; const run = resolveRun(db, params, res); if (!run) return; const input = await parseBody(req, res, reviewersInput); diff --git a/packages/cli/src/routes/pull-request-shared.ts b/packages/cli/src/routes/pull-request-shared.ts index 8faabb5..4a57e36 100644 --- a/packages/cli/src/routes/pull-request-shared.ts +++ b/packages/cli/src/routes/pull-request-shared.ts @@ -57,3 +57,28 @@ export function parseNumber(value: string | null): number | null { const n = Number(value); return Number.isInteger(n) && n > 0 ? n : null; } + +function isLoopbackOrigin(origin: string): boolean { + try { + const { hostname } = new URL(origin); + return hostname === "127.0.0.1" || hostname === "localhost" || hostname === "[::1]"; + } catch { + return false; + } +} + +/** + * Reject cross-origin state-changing requests (CSRF guard for the gh-backed + * mutations). The server binds to loopback, but a browser on any site can POST + * to the predictable port and trigger a write. Browsers always attach an + * accurate `Origin` on cross-origin requests and JS can't forge it, so we + * require a loopback origin. Non-browser clients (curl, scripts) send no + * `Origin` and are allowed — they aren't a CSRF vector. Returns false (and + * writes 403) when the request must be rejected. + */ +export function enforceSameOrigin(req: Req, res: Res): boolean { + const origin = req.headers.origin; + if (origin === undefined || isLoopbackOrigin(origin)) return true; + writeJson(res, 403, { error: "Cross-origin request rejected" }); + return false; +} From ff3eac081a61a22831f0c5f0c2425c8473f7f8df Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 1 Jun 2026 15:35:04 -0700 Subject: [PATCH 07/12] fix(cli): return 400 for malformed JSON mutation bodies parseBody now catches the SyntaxError readJsonBody throws on invalid JSON and responds 400 "Invalid request body" instead of letting it escape to the server's plain-text 500, so the client toasts the intended message. --- .../pull-request-mutations.routes.test.ts | 25 +++++++++++++++++++ .../cli/src/routes/pull-request-mutations.ts | 11 +++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts b/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts index b7ac9bc..d5c0528 100644 --- a/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts +++ b/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts @@ -225,4 +225,29 @@ describe("pull-request mutation API", () => { }); expect(res.status).toBe(400); }); + + it("returns 400 (not 500) for a malformed JSON body", async () => { + const runId = insertRun(); + const port = await start(); + const res = await new Promise<{ status: number }>((resolve, reject) => { + const req = http.request( + { + hostname: LOOPBACK_HOST, + port, + method: "POST", + path: `/api/runs/${runId}/pull-request/close`, + agent: false, + headers: { "Content-Type": "application/json" }, + }, + (r) => { + r.on("data", () => {}); + r.on("end", () => resolve({ status: r.statusCode ?? 0 })); + }, + ); + req.on("error", reject); + req.end("{ not valid json"); + }); + expect(res.status).toBe(400); + expect(await ghArgs()).toEqual([]); + }); }); diff --git a/packages/cli/src/routes/pull-request-mutations.ts b/packages/cli/src/routes/pull-request-mutations.ts index 2a4d8b9..ef56608 100644 --- a/packages/cli/src/routes/pull-request-mutations.ts +++ b/packages/cli/src/routes/pull-request-mutations.ts @@ -42,7 +42,16 @@ const autoMergeInput = z.object({ const reviewersInput = z.object({ number: numberField, reviewers: z.array(z.string()).min(1) }); async function parseBody(req: Req, res: Res, schema: z.ZodType): Promise { - const parsed = schema.safeParse(await readJsonBody(req)); + let raw: unknown; + try { + raw = await readJsonBody(req); + } catch { + // Malformed JSON throws inside readJsonBody — return 400 rather than letting + // it escape to the server's plain-text 500 catch-all. + writeJson(res, 400, { error: "Invalid request body" }); + return null; + } + const parsed = schema.safeParse(raw); if (!parsed.success) { writeJson(res, 400, { error: "Invalid request body" }); return null; From e9405fa3ec8754a4b0b0024a0133e2183ac062c0 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 1 Jun 2026 15:45:32 -0700 Subject: [PATCH 08/12] fix(web): surface gh error message on title + reviewer failures Title-edit and reviewer add/remove/re-request onError handlers now toast error.message when present (mirroring the merge and status actions), so the gh stderr preserved by write() reaches the user instead of a generic toast. --- .../components/pull-request/pull-request-header.tsx | 4 ++-- .../web/src/components/pull-request/reviewers.tsx | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/web/src/components/pull-request/pull-request-header.tsx b/packages/web/src/components/pull-request/pull-request-header.tsx index 100b8aa..ad9b7c1 100644 --- a/packages/web/src/components/pull-request/pull-request-header.tsx +++ b/packages/web/src/components/pull-request/pull-request-header.tsx @@ -104,10 +104,10 @@ export function PullRequestHeader({ pullRequest, mergeInfo }: PullRequestHeaderP setIsEditing(false); toast.success("Title updated"); }, - onError: () => { + onError: (error) => { setEditValue(pullRequest.title); setIsEditing(false); - toast.error("Failed to update title"); + toast.error(error instanceof Error ? error.message : "Failed to update title"); }, }); diff --git a/packages/web/src/components/pull-request/reviewers.tsx b/packages/web/src/components/pull-request/reviewers.tsx index 7240c4c..5742fd2 100644 --- a/packages/web/src/components/pull-request/reviewers.tsx +++ b/packages/web/src/components/pull-request/reviewers.tsx @@ -84,9 +84,9 @@ function ReviewerRow({ invalidatePullRequestQueries(); toast.success("Reviewer removed"); }, - onError: () => { + onError: (error) => { onRemoveError(reviewer.user.login); - toast.error("Failed to remove reviewer"); + toast.error(error instanceof Error ? error.message : "Failed to remove reviewer"); }, }); @@ -96,8 +96,8 @@ function ReviewerRow({ invalidatePullRequestQueries(); toast.success("Review re-requested"); }, - onError: () => { - toast.error("Failed to re-request review"); + onError: (error) => { + toast.error(error instanceof Error ? error.message : "Failed to re-request review"); }, }); @@ -228,9 +228,9 @@ function CollaboratorRow({ invalidatePullRequestQueries(); toast.success("Reviewer requested"); }, - onError: () => { + onError: (error) => { onAddError(user.login); - toast.error("Failed to add reviewer"); + toast.error(error instanceof Error ? error.message : "Failed to add reviewer"); }, }); From 83886cfa52369ef2611d08564d815c9cb921771c Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 1 Jun 2026 17:33:30 -0700 Subject: [PATCH 09/12] fix(cli): tighten PR-mutation CSRF guard to true same-origin enforceSameOrigin now requires the request Origin's host:port to match the Host it connected to, instead of accepting any loopback origin. This also rejects other local origins (e.g. a page on http://localhost:3000) that could otherwise POST to the predictable Stage port. Non-browser clients (no Origin) are still allowed. --- .../pull-request-mutations.routes.test.ts | 20 ++++++++++++++-- .../cli/src/routes/pull-request-shared.ts | 24 +++++++++---------- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts b/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts index d5c0528..cce8f6d 100644 --- a/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts +++ b/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts @@ -202,7 +202,7 @@ describe("pull-request mutation API", () => { expect(await ghArgs()).toEqual([]); }); - it("allows a mutation from a loopback origin", async () => { + it("rejects another local origin on a different port (CSRF guard)", async () => { const runId = insertRun(); const port = await start(); const res = await send( @@ -211,7 +211,23 @@ describe("pull-request mutation API", () => { `/api/runs/${runId}/pull-request/close`, { number: 7 }, { - Origin: `http://127.0.0.1:${port}`, + Origin: "http://localhost:3000", + }, + ); + expect(res.status).toBe(403); + expect(await ghArgs()).toEqual([]); + }); + + it("allows a same-origin mutation (Origin host matches the server)", async () => { + const runId = insertRun(); + const port = await start(); + const res = await send( + port, + "POST", + `/api/runs/${runId}/pull-request/close`, + { number: 7 }, + { + Origin: `http://${LOOPBACK_HOST}:${port}`, }, ); expect(res.status).toBe(200); diff --git a/packages/cli/src/routes/pull-request-shared.ts b/packages/cli/src/routes/pull-request-shared.ts index 4a57e36..3e618ca 100644 --- a/packages/cli/src/routes/pull-request-shared.ts +++ b/packages/cli/src/routes/pull-request-shared.ts @@ -58,27 +58,25 @@ export function parseNumber(value: string | null): number | null { return Number.isInteger(n) && n > 0 ? n : null; } -function isLoopbackOrigin(origin: string): boolean { - try { - const { hostname } = new URL(origin); - return hostname === "127.0.0.1" || hostname === "localhost" || hostname === "[::1]"; - } catch { - return false; - } -} - /** * Reject cross-origin state-changing requests (CSRF guard for the gh-backed * mutations). The server binds to loopback, but a browser on any site can POST * to the predictable port and trigger a write. Browsers always attach an * accurate `Origin` on cross-origin requests and JS can't forge it, so we - * require a loopback origin. Non-browser clients (curl, scripts) send no - * `Origin` and are allowed — they aren't a CSRF vector. Returns false (and - * writes 403) when the request must be rejected. + * require the request to be same-origin: the `Origin`'s host:port must match + * the `Host` it connected to. This rejects not just remote sites but other + * local origins too (e.g. a page on `http://localhost:3000`). Non-browser + * clients (curl, scripts) send no `Origin` and are allowed — they aren't a CSRF + * vector. Returns false (and writes 403) when the request must be rejected. */ export function enforceSameOrigin(req: Req, res: Res): boolean { const origin = req.headers.origin; - if (origin === undefined || isLoopbackOrigin(origin)) return true; + if (origin === undefined) return true; + try { + if (req.headers.host && new URL(origin).host === req.headers.host) return true; + } catch { + // malformed Origin — fall through to reject + } writeJson(res, 403, { error: "Cross-origin request rejected" }); return false; } From 2f5e157dbf5ea9f9d8c0e9928fa66eb149fa18a7 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 1 Jun 2026 18:22:18 -0700 Subject: [PATCH 10/12] feat(cli): surface preview deployment links in the PR header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getChecks now fetches the commit's deployments via a single gh api graphql query (commit.deployments + latestStatus), in parallel with check-runs, and resolves one link per environment — the latest successful deployment with an https URL (mirrors hosted Stage's resolveDeploymentLinks). The header's already-vendored deployment button/popover renders them with no UI change. Replaces the previous always-empty deploymentLinks (and the inaccurate "needs a GitHub App" note — the Deployments data is readable via gh). --- .../src/__tests__/pull-request.routes.test.ts | 53 +++++++- packages/cli/src/github/pull-request.ts | 124 +++++++++++++++--- 2 files changed, 161 insertions(+), 16 deletions(-) diff --git a/packages/cli/src/__tests__/pull-request.routes.test.ts b/packages/cli/src/__tests__/pull-request.routes.test.ts index d8a5900..92afe84 100644 --- a/packages/cli/src/__tests__/pull-request.routes.test.ts +++ b/packages/cli/src/__tests__/pull-request.routes.test.ts @@ -110,6 +110,37 @@ const MERGE_JSON = JSON.stringify({ }, }, }); +// Deployments GraphQL response (newest-first). Exercises dedupe-by-environment, +// skipping non-success and non-https/null URLs. +const DEPLOYMENTS_JSON = JSON.stringify({ + data: { + repository: { + object: { + deployments: { + nodes: [ + { + environment: "Preview", + latestStatus: { state: "SUCCESS", environmentUrl: "https://preview-2.example.app" }, + }, + { + environment: "Preview", + latestStatus: { state: "SUCCESS", environmentUrl: "https://preview-1.example.app" }, + }, + { + environment: "Production", + latestStatus: { state: "SUCCESS", environmentUrl: "https://prod.example.app" }, + }, + { + environment: "Staging", + latestStatus: { state: "FAILURE", environmentUrl: "https://staging.example.app" }, + }, + { environment: "NoUrl", latestStatus: { state: "SUCCESS", environmentUrl: null } }, + ], + }, + }, + }, + }, +}); beforeEach(async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "stage-cli-pr-routes-")); @@ -143,6 +174,7 @@ async function writeFakeGh(fixtures: { reviews?: string; checks?: string; merge?: string; + deployments?: string; }): Promise { const dir = path.join(binDir, "fixtures"); await fs.mkdir(dir, { recursive: true }); @@ -155,13 +187,15 @@ async function writeFakeGh(fixtures: { write("reviews.json", fixtures.reviews), write("checks.json", fixtures.checks), write("merge.json", fixtures.merge), + write("deployments.json", fixtures.deployments), ]); const script = `#!/bin/sh dir="${dir}" emit() { [ -f "$dir/$1" ] && cat "$dir/$1" || exit 1; } all="$*" if [ "$1" = "pr" ] && [ "$2" = "view" ]; then emit pr.json -elif [ "$1" = "api" ] && [ "$2" = "graphql" ]; then emit merge.json +elif [ "$1" = "api" ] && [ "$2" = "graphql" ]; then + case "$all" in *deployments*) emit deployments.json ;; *) emit merge.json ;; esac elif [ "$1" = "api" ]; then case "$all" in *check-runs*) emit checks.json ;; @@ -288,6 +322,23 @@ describe("pull-request API", () => { }); }); + it("returns one deployment link per environment (latest success, https only)", async () => { + await writeFakeGh({ checks: CHECKS_JSON, deployments: DEPLOYMENTS_JSON }); + const runId = insertRun(GITHUB_ORIGIN); + const res = await request( + await start(), + `/api/runs/${runId}/pull-request/checks?headSha=${SHA}`, + ); + expect(res.status).toBe(200); + const body = JSON.parse(res.body) as ChecksResponse; + // Preview deduped to the newest success; Production kept; Staging (failure) + // and NoUrl (null url) dropped. + expect(body.deploymentLinks).toEqual([ + { environment: "Preview", url: "https://preview-2.example.app" }, + { environment: "Production", url: "https://prod.example.app" }, + ]); + }); + it("rejects a checks request without a valid headSha", async () => { await writeFakeGh({ checks: CHECKS_JSON }); const runId = insertRun(GITHUB_ORIGIN); diff --git a/packages/cli/src/github/pull-request.ts b/packages/cli/src/github/pull-request.ts index b5914b6..bb60dea 100644 --- a/packages/cli/src/github/pull-request.ts +++ b/packages/cli/src/github/pull-request.ts @@ -3,6 +3,7 @@ import { CHECK_ITEM_SOURCE, type CheckItem, type ChecksResponse, + type DeploymentLink, type GitHubPullRequest, type GitHubUser, type MergeStatusInfo, @@ -206,20 +207,11 @@ function deriveCiState(items: CheckItem[]): ChecksResponse["state"] { return anyPending ? PULL_REQUEST_CI_STATUS.PENDING : PULL_REQUEST_CI_STATUS.SUCCESS; } -/** - * CI check runs for `headSha`. Deployment links require a GitHub App - * integration the CLI doesn't have, so `deploymentLinks` is always empty here. - */ -export async function getChecks( +async function getCheckRunItems( repoRoot: string, repo: GitHubRepo, headSha: string, -): Promise { - const empty: ChecksResponse = { - state: PULL_REQUEST_CI_STATUS.NONE, - items: [], - deploymentLinks: [], - }; +): Promise { try { // `--slurp` wraps every page into one JSON array (`[{page}, {page}, …]`); // without it, `--paginate` concatenates raw page objects, which isn't valid @@ -234,14 +226,116 @@ export async function getChecks( repoRoot, ); const parsed = z.array(GhCheckRunsSchema).safeParse(JSON.parse(stdout)); - if (!parsed.success) return empty; - const items = parsed.data.flatMap((page) => page.check_runs).map(toCheckItem); - return { state: deriveCiState(items), items, deploymentLinks: [] }; + if (!parsed.success) return []; + return parsed.data.flatMap((page) => page.check_runs).map(toCheckItem); } catch { - return empty; + return []; } } +// ─── Deployments ────────────────────────────────────────────────────────────── + +// Fetch the commit's deployments + each one's latest status in a single query, +// newest-first, so dedupe-by-environment below keeps the most recent per env. +const DEPLOYMENTS_QUERY = `query GetDeployments($owner: String!, $repo: String!, $sha: GitObjectID!) { + repository(owner: $owner, name: $repo) { + object(oid: $sha) { + ... on Commit { + deployments(first: 20, orderBy: { field: CREATED_AT, direction: DESC }) { + nodes { environment latestStatus { state environmentUrl } } + } + } + } + } +}`; + +const GhDeploymentsSchema = z.object({ + data: z.object({ + repository: z + .object({ + object: z + .object({ + deployments: z.object({ + nodes: z.array( + z.object({ + environment: z.string(), + latestStatus: z + .object({ state: z.string(), environmentUrl: z.string().nullable() }) + .nullable(), + }), + ), + }), + }) + .nullable(), + }) + .nullable(), + }), +}); + +/** + * Preview/deployment links for `headSha`: one per environment, keeping the + * latest successful deployment with an https URL (mirrors hosted Stage's + * resolveDeploymentLinks). Returns [] on any failure so checks still render. + */ +async function getDeploymentLinks( + repoRoot: string, + repo: GitHubRepo, + headSha: string, +): Promise { + try { + const stdout = await gh( + [ + "api", + "graphql", + "-f", + `query=${DEPLOYMENTS_QUERY}`, + "-F", + `owner=${repo.owner}`, + "-F", + `repo=${repo.repo}`, + "-F", + `sha=${headSha}`, + ], + repoRoot, + ); + const parsed = GhDeploymentsSchema.safeParse(JSON.parse(stdout)); + if (!parsed.success) return []; + const nodes = parsed.data.data.repository?.object?.deployments.nodes ?? []; + const byEnvironment = new Map(); + for (const node of nodes) { + const url = node.latestStatus?.environmentUrl; + if ( + node.latestStatus?.state === "SUCCESS" && + url && + url.startsWith("https://") && + !byEnvironment.has(node.environment) + ) { + byEnvironment.set(node.environment, { environment: node.environment, url }); + } + } + return [...byEnvironment.values()]; + } catch { + return []; + } +} + +/** + * CI check runs plus preview-deployment links for `headSha`. Runs the two gh + * reads in parallel; each degrades to empty independently so a failure in one + * never blanks the other. + */ +export async function getChecks( + repoRoot: string, + repo: GitHubRepo, + headSha: string, +): Promise { + const [items, deploymentLinks] = await Promise.all([ + getCheckRunItems(repoRoot, repo, headSha), + getDeploymentLinks(repoRoot, repo, headSha), + ]); + return { state: deriveCiState(items), items, deploymentLinks }; +} + // ─── Reviews ──────────────────────────────────────────────────────────────── // REST reviews are returned oldest-first, so iterating and overwriting per login From 681053adfb208390ae0fcf744a8b24bec445e1c0 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 1 Jun 2026 18:42:46 -0700 Subject: [PATCH 11/12] fix(web): forward head SHA to auto-merge enable paths setAutoMergeMutationOptions dropped expectedHeadOid, so enabling auto-merge never sent --match-head-commit even though the server, merge, and enqueue paths all support it. Forward headSha from MergeActions on both enable sites (merge-queue not-ready and non-merge-queue toggle); disabling still omits it. --- .../web/src/components/pull-request/merge-status.tsx | 11 +++++++++-- packages/web/src/lib/pull-request-mutations.ts | 10 +++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/web/src/components/pull-request/merge-status.tsx b/packages/web/src/components/pull-request/merge-status.tsx index f6c5b23..9312013 100644 --- a/packages/web/src/components/pull-request/merge-status.tsx +++ b/packages/web/src/components/pull-request/merge-status.tsx @@ -172,7 +172,14 @@ function MergeActions({ if (isReady) { enqueueMutation.mutate({ owner, repo, number, expectedHeadOid: headSha }); } else { - autoMergeMutation.mutate({ owner, repo, number, enabled: true, mergeMethod }); + autoMergeMutation.mutate({ + owner, + repo, + number, + enabled: true, + mergeMethod, + expectedHeadOid: headSha, + }); } } else if (mergeInfo.isInMergeQueue && mergeInfo.entry) { dequeueMutation.mutate({ @@ -213,7 +220,7 @@ function MergeActions({ repo, number, enabled: checked, - ...(checked && { mergeMethod }), + ...(checked && { mergeMethod, expectedHeadOid: headSha }), }); }} /> diff --git a/packages/web/src/lib/pull-request-mutations.ts b/packages/web/src/lib/pull-request-mutations.ts index 04cc9f1..099a574 100644 --- a/packages/web/src/lib/pull-request-mutations.ts +++ b/packages/web/src/lib/pull-request-mutations.ts @@ -115,12 +115,20 @@ export function enqueueMutationOptions(runId: string) { export function setAutoMergeMutationOptions(runId: string) { return { mutationFn: ( - v: RepoVars & { number: number; enabled: boolean; mergeMethod?: PullRequestMergeMethod }, + v: RepoVars & { + number: number; + enabled: boolean; + mergeMethod?: PullRequestMergeMethod; + // Forward the head SHA so enabling auto-merge guards against a stale head + // (--match-head-commit). The server ignores it when disabling. + expectedHeadOid?: string; + }, ) => write(runId, "/auto-merge", "POST", { number: v.number, enabled: v.enabled, mergeMethod: v.mergeMethod, + expectedHeadOid: v.expectedHeadOid, }), }; } From b38295124864686edaaf448b9999fe5f0b770864 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 1 Jun 2026 19:36:31 -0700 Subject: [PATCH 12/12] fix(cli): block DNS rebinding on PR mutations via loopback-Host check The same-origin guard compared Origin host to Host, but DNS rebinding defeats that: an attacker page rebound to 127.0.0.1 sends its own hostname in both headers, so they match. The server only ever binds to loopback, so require the Host header's hostname to be a loopback literal (127.0.0.1/localhost) before the same-origin check. Adds a rebinding test (matching Origin + non-loopback Host). --- .../pull-request-mutations.routes.test.ts | 20 ++++++++ .../cli/src/routes/pull-request-shared.ts | 51 +++++++++++++++---- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts b/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts index cce8f6d..2d11692 100644 --- a/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts +++ b/packages/cli/src/__tests__/pull-request-mutations.routes.test.ts @@ -218,6 +218,26 @@ describe("pull-request mutation API", () => { expect(await ghArgs()).toEqual([]); }); + it("rejects a DNS-rebinding mutation (matching Origin + non-loopback Host)", async () => { + const runId = insertRun(); + const port = await start(); + // DNS rebinding: the attacker's page is served from attacker.example:PORT, + // rebound to 127.0.0.1, so the browser sends that hostname in BOTH Origin and + // Host — a bare Origin===Host check would pass. The loopback-Host guard rejects. + const res = await send( + port, + "POST", + `/api/runs/${runId}/pull-request/close`, + { number: 7 }, + { + Origin: `http://attacker.example:${port}`, + Host: `attacker.example:${port}`, + }, + ); + expect(res.status).toBe(403); + expect(await ghArgs()).toEqual([]); + }); + it("allows a same-origin mutation (Origin host matches the server)", async () => { const runId = insertRun(); const port = await start(); diff --git a/packages/cli/src/routes/pull-request-shared.ts b/packages/cli/src/routes/pull-request-shared.ts index 3e618ca..6f4dcad 100644 --- a/packages/cli/src/routes/pull-request-shared.ts +++ b/packages/cli/src/routes/pull-request-shared.ts @@ -58,22 +58,53 @@ export function parseNumber(value: string | null): number | null { return Number.isInteger(n) && n > 0 ? n : null; } +/** The server only ever binds to loopback, so a legitimate `Host` is one of these. */ +const LOOPBACK_HOSTNAMES = new Set(["127.0.0.1", "localhost"]); + +/** Hostname from a `Host` header (`host[:port]`), or null if it can't be parsed. */ +function hostHeaderHostname(host: string | undefined): string | null { + if (!host) return null; + try { + return new URL(`http://${host}`).hostname; + } catch { + return null; + } +} + /** - * Reject cross-origin state-changing requests (CSRF guard for the gh-backed - * mutations). The server binds to loopback, but a browser on any site can POST - * to the predictable port and trigger a write. Browsers always attach an - * accurate `Origin` on cross-origin requests and JS can't forge it, so we - * require the request to be same-origin: the `Origin`'s host:port must match - * the `Host` it connected to. This rejects not just remote sites but other - * local origins too (e.g. a page on `http://localhost:3000`). Non-browser - * clients (curl, scripts) send no `Origin` and are allowed — they aren't a CSRF - * vector. Returns false (and writes 403) when the request must be rejected. + * Reject state-changing requests that could be CSRF or DNS-rebinding vectors + * against the gh-backed mutations. The server binds to loopback, but a browser + * on any site can POST to the predictable port and trigger a write. + * + * Two checks, both required: + * + * 1. **Loopback Host.** The server only binds to `127.0.0.1`, so a legitimate + * request always carries a loopback `Host`. This is the anti-DNS-rebinding + * guard: an attacker who rebinds their hostname to `127.0.0.1` reaches us + * with that hostname in *both* `Origin` and `Host` — defeating a bare + * `Origin === Host` check — but the hostname isn't a loopback literal, so we + * reject it here. `Host` is mandatory under HTTP/1.1, so this also rejects + * requests that omit it. + * 2. **Same-origin.** Browsers always attach an accurate `Origin` on + * cross-origin requests and JS can't forge it, so when an `Origin` is present + * its host:port must match the `Host` connected to. This rejects remote sites + * and other local origins alike (e.g. a page on `http://localhost:3000`). + * + * Non-browser clients (curl, scripts) send no `Origin` and are allowed once the + * Host check passes — they aren't a CSRF vector. Returns false (and writes 403) + * when the request must be rejected. */ export function enforceSameOrigin(req: Req, res: Res): boolean { + const host = req.headers.host; + const hostname = hostHeaderHostname(host); + if (hostname === null || !LOOPBACK_HOSTNAMES.has(hostname)) { + writeJson(res, 403, { error: "Cross-origin request rejected" }); + return false; + } const origin = req.headers.origin; if (origin === undefined) return true; try { - if (req.headers.host && new URL(origin).host === req.headers.host) return true; + if (new URL(origin).host === host) return true; } catch { // malformed Origin — fall through to reject }