From b871046ad272c42cd08cbac29ef2513a6c11d745 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 5 Jun 2026 19:37:59 -0700 Subject: [PATCH] =?UTF-8?q?feat(pr):=20GitHub=20primitives=20=E2=80=94=20t?= =?UTF-8?q?hread=20reply/resolve,=20ETag=20probe,=20expectedHeadOid=20merg?= =?UTF-8?q?e=20(U2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the net-new GitHub primitives the PR nodes need: replyToReviewThread and resolveReviewThread (GraphQL mutations, dual gh/API), an ETag-conditional probePrChanged (304 is rate-limit-free, gates the reconcile deep-fetch), and expectedHeadOid on mergePr (--match-head-commit / REST sha) raising a typed PrStaleHeadError on a head-moved race so pr-merge can re-evaluate. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/__tests__/github-pr-threads.test.ts | 93 ++++++++++++++ packages/dashboard/src/github.ts | 121 +++++++++++++++++- 2 files changed, 211 insertions(+), 3 deletions(-) create mode 100644 packages/dashboard/src/__tests__/github-pr-threads.test.ts diff --git a/packages/dashboard/src/__tests__/github-pr-threads.test.ts b/packages/dashboard/src/__tests__/github-pr-threads.test.ts new file mode 100644 index 0000000000..ea91d90a4b --- /dev/null +++ b/packages/dashboard/src/__tests__/github-pr-threads.test.ts @@ -0,0 +1,93 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@fusion/core", async () => { + const actual = await vi.importActual("@fusion/core"); + return { + ...actual, + isGhAvailable: vi.fn(() => true), + isGhAuthenticated: vi.fn(() => true), + runGh: vi.fn(), + runGhAsync: vi.fn(), + runGhJson: vi.fn(), + runGhJsonAsync: vi.fn(), + getGhErrorMessage: vi.fn((err) => (err instanceof Error ? err.message : String(err))), + getCurrentRepo: vi.fn(() => ({ owner: "owner", repo: "repo" })), + }; +}); + +import { runGh, runGhAsync, runGhJsonAsync, isGhAvailable, isGhAuthenticated } from "@fusion/core"; +import { GitHubClient, PrStaleHeadError } from "../github.js"; + +const mockRunGh = vi.mocked(runGh); +const mockRunGhAsync = vi.mocked(runGhAsync); +const mockRunGhJsonAsync = vi.mocked(runGhJsonAsync); + +const prView = { + number: 42, + url: "https://github.com/owner/repo/pull/42", + title: "T", + state: "OPEN", + isDraft: false, + baseRefName: "main", + headRefName: "fusion/t-1", +}; + +describe("GitHubClient PR thread + merge primitives (U2)", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(isGhAvailable).mockReturnValue(true); + vi.mocked(isGhAuthenticated).mockReturnValue(true); + }); + + it("replies to a review thread via the GraphQL mutation", async () => { + mockRunGhAsync.mockResolvedValue(JSON.stringify({ data: { addPullRequestReviewThreadReply: { comment: { id: "c1" } } } })); + const client = new GitHubClient({ forceMode: "gh-cli" }); + await client.replyToReviewThread("THREAD_1", "thanks, fixed in abc123"); + const args = mockRunGhAsync.mock.calls[0][0] as string[]; + expect(args.slice(0, 2)).toEqual(["api", "graphql"]); + expect(args.join(" ")).toContain("addPullRequestReviewThreadReply"); + expect(args).toContain("threadId=THREAD_1"); + }); + + it("resolves a review thread via the GraphQL mutation", async () => { + mockRunGhAsync.mockResolvedValue(JSON.stringify({ data: { resolveReviewThread: { thread: { id: "t", isResolved: true } } } })); + const client = new GitHubClient({ forceMode: "gh-cli" }); + await client.resolveReviewThread("THREAD_2"); + const args = mockRunGhAsync.mock.calls[0][0] as string[]; + expect(args.join(" ")).toContain("resolveReviewThread"); + expect(args).toContain("threadId=THREAD_2"); + }); + + it("surfaces a GraphQL error from a thread mutation", async () => { + mockRunGhAsync.mockResolvedValue(JSON.stringify({ errors: [{ message: "Thread is locked" }] })); + const client = new GitHubClient({ forceMode: "gh-cli" }); + await expect(client.replyToReviewThread("T", "x")).rejects.toThrow("Thread is locked"); + }); + + it("passes --match-head-commit when expectedHeadOid is set", async () => { + mockRunGh.mockReturnValue("" as never); + mockRunGhJsonAsync.mockResolvedValue(prView as never); + const client = new GitHubClient({ forceMode: "gh-cli" }); + await client.mergePr({ number: 42, expectedHeadOid: "deadbeef" }); + const args = mockRunGh.mock.calls[0][0] as string[]; + expect(args).toContain("--match-head-commit"); + expect(args).toContain("deadbeef"); + }); + + it("raises PrStaleHeadError when the head moved (gh path)", async () => { + mockRunGh.mockImplementation(() => { + throw new Error("failed to merge: Head branch was modified. Review and try the merge again."); + }); + const client = new GitHubClient({ forceMode: "gh-cli" }); + await expect(client.mergePr({ number: 42, expectedHeadOid: "deadbeef" })).rejects.toBeInstanceOf(PrStaleHeadError); + }); + + it("does not request a head match when expectedHeadOid is absent", async () => { + mockRunGh.mockReturnValue("" as never); + mockRunGhJsonAsync.mockResolvedValue(prView as never); + const client = new GitHubClient({ forceMode: "gh-cli" }); + await client.mergePr({ number: 42 }); + const args = mockRunGh.mock.calls[0][0] as string[]; + expect(args).not.toContain("--match-head-commit"); + }); +}); diff --git a/packages/dashboard/src/github.ts b/packages/dashboard/src/github.ts index 884b311488..27b5bdfd65 100644 --- a/packages/dashboard/src/github.ts +++ b/packages/dashboard/src/github.ts @@ -217,6 +217,21 @@ export interface MergePrParams { repo?: string; number: number; method?: "merge" | "squash" | "rebase"; + /** + * When set, the merge only proceeds if the PR head still points at this SHA + * (defeats the push/merge race — U2/U6). A mismatch surfaces as + * PrStaleHeadError so the pr-merge node can re-evaluate against the new head. + */ + expectedHeadOid?: string; +} + +/** Thrown when a merge is rejected because the PR head moved (expectedHeadOid mismatch). */ +export class PrStaleHeadError extends Error { + readonly code = "stale-head" as const; + constructor(message: string) { + super(message); + this.name = "PrStaleHeadError"; + } } export interface UpdatePrParams { @@ -1801,6 +1816,9 @@ export class GitHubClient { try { return await this.mergePrWithGh(params); } catch (err) { + // A stale-head rejection is a real outcome, not a gh-vs-API fallback + // trigger — re-running on the API path would merge the wrong head. + if (err instanceof PrStaleHeadError) throw err; if (this.token) { return this.mergePrWithApi(params); } @@ -1816,34 +1834,131 @@ export class GitHubClient { private async mergePrWithGh(params: MergePrParams): Promise { const resolved = this.resolveRepo(params.owner, params.repo); - runGh([ + const args = [ "pr", "merge", String(params.number), "--repo", `${resolved.owner}/${resolved.repo}`, `--${params.method ?? "squash"}`, "--delete-branch", - ]); + ]; + if (params.expectedHeadOid) { + args.push("--match-head-commit", params.expectedHeadOid); + } + try { + runGh(args); + } catch (err) { + const message = getGhErrorMessage(err); + if ( + params.expectedHeadOid && + /head.*(changed|modified|match|stale)|not the most recent|base branch was modified/i.test(message) + ) { + throw new PrStaleHeadError(`PR #${params.number} head moved since ${params.expectedHeadOid}; merge aborted`); + } + throw err; + } return this.getPrStatus(resolved.owner, resolved.repo, params.number); } private async mergePrWithApi(params: MergePrParams): Promise { const resolved = this.resolveRepo(params.owner, params.repo); + const body: Record = { merge_method: params.method ?? "squash" }; + if (params.expectedHeadOid) body.sha = params.expectedHeadOid; const response = await fetch( `${this.baseUrl}/repos/${encodeURIComponent(resolved.owner)}/${encodeURIComponent(resolved.repo)}/pulls/${params.number}/merge`, { method: "PUT", headers: this.buildHeaders(), - body: JSON.stringify({ merge_method: params.method ?? "squash" }), + body: JSON.stringify(body), }, ); if (!response.ok) { const error = await response.json().catch(() => ({ message: response.statusText })); + // 409 Conflict with a `sha` set means the head moved (stale-head race). + if (params.expectedHeadOid && response.status === 409) { + throw new PrStaleHeadError(`PR #${params.number} head moved since ${params.expectedHeadOid}; merge aborted`); + } throw new Error(`GitHub API error: ${response.status} ${error.message || response.statusText}`); } return this.getPrStatus(resolved.owner, resolved.repo, params.number); } + /** + * Reply to a specific review thread (U2). GraphQL only — REST has no + * thread-level reply that also carries thread identity. Honors viewerCanReply + * by surfacing GitHub's error rather than guessing. + */ + async replyToReviewThread(threadId: string, body: string): Promise { + const query = `mutation($threadId: ID!, $body: String!) { + addPullRequestReviewThreadReply(input: { pullRequestReviewThreadId: $threadId, body: $body }) { + comment { id } + } + }`; + await this.runGraphqlMutation(query, { threadId, body }); + } + + /** Resolve a review thread (U2). GraphQL only; caller should check viewerCanResolve first. */ + async resolveReviewThread(threadId: string): Promise { + const query = `mutation($threadId: ID!) { + resolveReviewThread(input: { threadId: $threadId }) { thread { id isResolved } } + }`; + await this.runGraphqlMutation(query, { threadId }); + } + + /** + * ETag-conditional change probe (U2/U17). Returns { changed, etag } so the + * reconcile can skip the expensive GraphQL deep-fetch when GitHub reports 304 + * (which does not count against the primary rate limit). Only available on the + * REST/token path — gh CLI does not expose conditional requests. + */ + async probePrChanged( + owner: string | undefined, + repo: string | undefined, + number: number, + etag?: string, + ): Promise<{ changed: boolean; etag?: string }> { + if (!this.token) { + // No conditional-request path without a token; treat as always-changed so + // the caller falls back to a full fetch. + return { changed: true }; + } + const resolved = this.resolveRepo(owner, repo); + const headers: Record = { ...this.buildHeaders() }; + if (etag) headers["If-None-Match"] = etag; + const response = await fetch( + `${this.baseUrl}/repos/${encodeURIComponent(resolved.owner)}/${encodeURIComponent(resolved.repo)}/pulls/${number}`, + { headers }, + ); + if (response.status === 304) return { changed: false, etag }; + return { changed: true, etag: response.headers.get("etag") ?? undefined }; + } + + private async runGraphqlMutation(query: string, variables: Record): Promise { + if (this.hasGhAuth()) { + const args = ["api", "graphql", "-f", `query=${query}`]; + for (const [key, value] of Object.entries(variables)) { + args.push("-F", `${key}=${value}`); + } + const output = await runGhAsync(args); + const payload = JSON.parse(output) as { errors?: Array<{ message: string }> }; + if (payload.errors?.length) throw new Error(payload.errors[0].message); + return; + } + if (this.token) { + const response = await fetch(`${this.baseUrl}/graphql`, { + method: "POST", + headers: { ...this.buildHeaders(), "Content-Type": "application/json" }, + body: JSON.stringify({ query, variables }), + }); + const payload = (await response.json()) as { errors?: Array<{ message: string }> }; + if (!response.ok || payload.errors?.length) { + throw new Error(`GitHub API error: ${response.status} ${payload.errors?.[0]?.message || response.statusText}`); + } + return; + } + throw new Error("GitHub CLI (gh) is not available or not authenticated, and no GITHUB_TOKEN provided."); + } + /** * Fetch current PR status using gh CLI if available, otherwise REST API. */