feat(pr): GitHub primitives — thread reply/resolve, ETag probe, expectedHeadOid merge (U2)
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) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
93
packages/dashboard/src/__tests__/github-pr-threads.test.ts
Normal file
93
packages/dashboard/src/__tests__/github-pr-threads.test.ts
Normal file
@@ -0,0 +1,93 @@
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
vi.mock("@fusion/core", async () => {
|
||||
const actual = await vi.importActual<typeof import("@fusion/core")>("@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");
|
||||
});
|
||||
});
|
||||
@@ -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<PrInfo> {
|
||||
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<PrInfo> {
|
||||
const resolved = this.resolveRepo(params.owner, params.repo);
|
||||
const body: Record<string, string> = { 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<void> {
|
||||
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<void> {
|
||||
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<string, string> = { ...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<string, string>): Promise<void> {
|
||||
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.
|
||||
*/
|
||||
|
||||
Reference in New Issue
Block a user