diff --git a/.changeset/fn-6960-pr-metadata-bounded.md b/.changeset/fn-6960-pr-metadata-bounded.md new file mode 100644 index 0000000000..8b6e5d5bc1 --- /dev/null +++ b/.changeset/fn-6960-pr-metadata-bounded.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Prevent Create PR metadata generation from hanging and provide editable fallback content. +category: fix +dev: Bounds PR metadata generation and validates non-empty PR bodies before GitHub PR creation. diff --git a/packages/dashboard/app/components/PrCreateModal.tsx b/packages/dashboard/app/components/PrCreateModal.tsx index 5f7313d8bb..52fb033d38 100644 --- a/packages/dashboard/app/components/PrCreateModal.tsx +++ b/packages/dashboard/app/components/PrCreateModal.tsx @@ -39,6 +39,30 @@ type PreflightCheck = { warning?: boolean; }; +/* +FNXC:PrCreateModal 2026-06-23-00:00: +The Create PR modal must stay manually usable after metadata generation fails, but non-interactive GitHub PR creation cannot accept a title-only payload. Seed an editable body fallback with the required sections so users can complete or revise the PR instead of submitting an empty body. +*/ +function buildManualPrBodyFallback(taskId: string): string { + return [ + "## Summary", + "", + "Summary unavailable. Add context before creating this PR.", + "", + "## Changes", + "", + "- Details unavailable.", + "", + "## Testing", + "", + "- Not provided.", + "", + "## Linked Task", + "", + `Closes ${taskId}`, + ].join("\n"); +} + function OptionChips( { label, @@ -208,6 +232,8 @@ export function PrCreateModal({ } catch (loadError) { if (requestId === requestSeqRef.current.metadata) { setMetadataError(getErrorMessage(loadError)); + setBody((current) => (current.trim() ? current : buildManualPrBodyFallback(taskId))); + setAiBody((current) => current || buildManualPrBodyFallback(taskId)); } } finally { if (requestId === requestSeqRef.current.metadata) { @@ -351,6 +377,8 @@ export function PrCreateModal({ } catch (regenerateError) { if (requestId === requestSeqRef.current.metadata) { setMetadataError(getErrorMessage(regenerateError)); + setBody((current) => (current.trim() ? current : buildManualPrBodyFallback(taskId))); + setAiBody((current) => current || buildManualPrBodyFallback(taskId)); } } finally { if (requestId === requestSeqRef.current.metadata) { @@ -445,7 +473,7 @@ export function PrCreateModal({ }), [assignees, baseBranch, body, draft, labels, reviewers, title]); const submit = useCallback(async () => { - if (!payload.title || submitting) return; + if (!payload.title || !payload.body || submitting) return; setSubmitting(true); setSubmitError(null); setLastGhError(null); @@ -466,6 +494,8 @@ export function PrCreateModal({ } }, [addToast, onClose, onCreated, payload, projectId, submitting, taskId]); + const hasRequiredPrContent = title.trim().length > 0 && body.trim().length > 0; + if (!open) return null; return createPortal( @@ -672,7 +702,7 @@ export function PrCreateModal({
- diff --git a/packages/dashboard/app/components/__tests__/PrCreateModal.test.tsx b/packages/dashboard/app/components/__tests__/PrCreateModal.test.tsx index 8c4f71aed1..448445cd03 100644 --- a/packages/dashboard/app/components/__tests__/PrCreateModal.test.tsx +++ b/packages/dashboard/app/components/__tests__/PrCreateModal.test.tsx @@ -182,6 +182,8 @@ describe("PrCreateModal", () => { expect(screen.getByRole("button", { name: "Create PR" })).toBeDisabled(); fireEvent.change(screen.getByLabelText(/title/i), { target: { value: "Manual PR title" } }); + expect(screen.getByRole("button", { name: "Create PR" })).toBeDisabled(); + fireEvent.change(screen.getByLabelText(/body/i), { target: { value: "Manual PR body" } }); await waitFor(() => expect(screen.getByRole("button", { name: "Create PR" })).toBeEnabled()); fireEvent.change(screen.getByPlaceholderText("Filter reviewers"), { target: { value: "rev" } }); @@ -215,10 +217,49 @@ describe("PrCreateModal", () => { expect(screen.getByText("Branch pushed to remote")).toBeInTheDocument(); expect(screen.getByLabelText(/base branch/i)).toBeEnabled(); + const bodyInput = screen.getByLabelText(/body/i) as HTMLTextAreaElement; + expect(bodyInput.value).toContain("## Summary"); + expect(bodyInput.value).toContain("## Changes"); + expect(bodyInput.value).toContain("## Testing"); + expect(bodyInput.value).toContain("Closes FN-4756"); + fireEvent.change(screen.getByLabelText(/title/i), { target: { value: "Manual fallback title" } }); await waitFor(() => expect(screen.getByRole("button", { name: "Create PR" })).toBeEnabled()); }); + it("requires a non-empty body before submitting manual PR metadata", async () => { + mocks.generatePrMetadata.mockRejectedValueOnce(new Error("metadata blew up")); + renderModal(); + + expect(await screen.findByText("metadata blew up")).toBeInTheDocument(); + fireEvent.change(screen.getByLabelText(/title/i), { target: { value: "Manual fallback title" } }); + await waitFor(() => expect(screen.getByRole("button", { name: "Create PR" })).toBeEnabled()); + + fireEvent.change(screen.getByLabelText(/body/i), { target: { value: "" } }); + expect(screen.getByRole("button", { name: "Create PR" })).toBeDisabled(); + fireEvent.click(screen.getByRole("button", { name: "Create PR" })); + expect(mocks.createPr).not.toHaveBeenCalled(); + + fireEvent.change(screen.getByLabelText(/body/i), { target: { value: "Manual fallback body" } }); + await waitFor(() => expect(screen.getByRole("button", { name: "Create PR" })).toBeEnabled()); + }); + + it("uses route fallback metadata as editable content after a timeout response", async () => { + mocks.generatePrMetadata.mockResolvedValueOnce({ + title: "Fallback title", + body: "## Summary\n\nSummary unavailable.\n\n## Changes\n\n- Details unavailable.\n\n## Testing\n\n- Not provided.\n\n## Linked Task\n\nCloses FN-4756", + templateUsed: false, + }); + renderModal(); + + expect(await screen.findByDisplayValue("Fallback title")).toBeInTheDocument(); + const bodyInput = screen.getByLabelText(/body/i) as HTMLTextAreaElement; + expect(bodyInput.value).toContain("## Summary"); + expect(bodyInput.value).toContain("Closes FN-4756"); + expect(screen.queryByText(/generating ai title/i)).not.toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Create PR" })).toBeEnabled(); + }); + it("keeps options failure scoped so metadata and preflight still render", async () => { mocks.fetchPrOptions.mockRejectedValueOnce(new Error("options blew up")); renderModal(); diff --git a/packages/dashboard/src/__tests__/github-create-pr.test.ts b/packages/dashboard/src/__tests__/github-create-pr.test.ts index c243a8eb36..5df0165f69 100644 --- a/packages/dashboard/src/__tests__/github-create-pr.test.ts +++ b/packages/dashboard/src/__tests__/github-create-pr.test.ts @@ -32,7 +32,7 @@ describe("GitHubClient.createPr draft/reviewer", () => { mockRunGh.mockReturnValue("https://github.com/owner/repo/pull/42\n"); const client = new GitHubClient({ forceMode: "gh-cli" }); - await client.createPr({ title: "T", head: "fusion/fn-001", base: "main", draft, reviewers }); + await client.createPr({ title: "T", body: "B", head: "fusion/fn-001", base: "main", draft, reviewers }); const args = mockRunGh.mock.calls[0][0]; expect(args.includes("--draft")).toBe(expectsDraft); @@ -48,7 +48,7 @@ describe("GitHubClient.createPr draft/reviewer", () => { } as any) .mockResolvedValueOnce({ ok: true, json: async () => ({}) } as any); - await client.createPr({ title: "T", head: "fusion/fn-001", base: "main", draft: true, reviewers: ["alice", "bob"] }); + await client.createPr({ title: "T", body: "B", head: "fusion/fn-001", base: "main", draft: true, reviewers: ["alice", "bob"] }); expect(String(fetchSpy.mock.calls[0][1]?.body)).toContain('"draft":true'); expect(fetchSpy.mock.calls[1][0]).toContain("/requested_reviewers"); @@ -65,7 +65,7 @@ describe("GitHubClient.createPr draft/reviewer", () => { } as any) .mockResolvedValueOnce({ ok: false, status: 422, statusText: "Unprocessable", json: async () => ({ message: "invalid reviewers" }) } as any); - const pr = await client.createPr({ title: "T", head: "fusion/fn-001", reviewers: ["alice"] }); + const pr = await client.createPr({ title: "T", body: "B", head: "fusion/fn-001", reviewers: ["alice"] }); expect(pr.number).toBe(8); expect(stderrSpy).toHaveBeenCalledWith(expect.stringContaining("failed to request reviewers")); diff --git a/packages/dashboard/src/__tests__/github-forced-mode.test.ts b/packages/dashboard/src/__tests__/github-forced-mode.test.ts index 1828a56916..425169b0b1 100644 --- a/packages/dashboard/src/__tests__/github-forced-mode.test.ts +++ b/packages/dashboard/src/__tests__/github-forced-mode.test.ts @@ -94,7 +94,7 @@ describe("GitHubClient forced mode", () => { } as never); const client = new GitHubClient("token-legacy"); - await client.createPr({ owner: "o", repo: "r", title: "t", head: "head", base: "main" }); + await client.createPr({ owner: "o", repo: "r", title: "t", body: "b", head: "head", base: "main" }); expect(mockRunGh).toHaveBeenCalledTimes(1); expect(fetchSpy).toHaveBeenCalledTimes(1); diff --git a/packages/dashboard/src/__tests__/github.test.ts b/packages/dashboard/src/__tests__/github.test.ts index f41b360b34..67d5a74eeb 100644 --- a/packages/dashboard/src/__tests__/github.test.ts +++ b/packages/dashboard/src/__tests__/github.test.ts @@ -225,28 +225,19 @@ describe("GitHubClient", () => { expect(result.status).toBe("open"); }); - it("creates PR without body when not provided", async () => { - mockRunGh.mockReturnValue("https://github.com/test-owner/test-repo/pull/42\n"); + it("rejects PR creation without a non-empty body before invoking gh", async () => { const paramsWithoutBody: CreatePrParams = { owner: "test-owner", repo: "test-repo", title: "Test PR", head: "feature-branch", - // body and base not provided }; - await client.createPr(paramsWithoutBody); + await expect(client.createPr(paramsWithoutBody)).rejects.toThrow("PR body is required"); + expect(mockRunGh).not.toHaveBeenCalled(); - expect(mockRunGh).toHaveBeenCalledWith([ - "pr", "create", - "--repo", "test-owner/test-repo", - "--title", "Test PR", - "--head", "feature-branch", - ]); - // Should not include --body or --base when not provided - const callArgs = mockRunGh.mock.calls[0][0]; - expect(callArgs).not.toContain("--body"); - expect(callArgs).not.toContain("--base"); + await expect(client.createPr({ ...paramsWithoutBody, body: " " })).rejects.toThrow("PR body is required"); + expect(mockRunGh).not.toHaveBeenCalled(); }); it("uses current repo context when owner/repo not specified", async () => { @@ -255,6 +246,7 @@ describe("GitHubClient", () => { const paramsWithoutRepo = { title: "Test PR", + body: "Test body", head: "feature-branch", }; @@ -266,6 +258,7 @@ describe("GitHubClient", () => { "--repo", "current-owner/current-repo", "--title", "Test PR", "--head", "feature-branch", + "--body", "Test body", ]); expect(result.number).toBe(5); }); diff --git a/packages/dashboard/src/__tests__/pr-metadata-generator.test.ts b/packages/dashboard/src/__tests__/pr-metadata-generator.test.ts index f0a8c64805..b5b1d33e2f 100644 --- a/packages/dashboard/src/__tests__/pr-metadata-generator.test.ts +++ b/packages/dashboard/src/__tests__/pr-metadata-generator.test.ts @@ -57,6 +57,14 @@ function setupExec(outputs: Record) { }); } +function expectFallbackBody(body: string) { + expect(body).toContain("## Summary"); + expect(body).toContain("## Changes"); + expect(body).toContain("## Testing"); + expect(body).toContain("## Linked Task"); + expect(body).toContain("Closes FN-4991"); +} + describe("generatePrMetadata", () => { let repoRoot: string; @@ -167,11 +175,12 @@ describe("generatePrMetadata", () => { body: expect.stringContaining("Closes FN-4991"), templateUsed: false, }); + expectFallbackBody(result.body); }); - it("returns fallback when prompt times out", async () => { + it("returns fallback when createFnAgent times out before a session exists", async () => { vi.useFakeTimers(); - promptMock.mockImplementation(() => new Promise(() => undefined)); + vi.mocked(createFnAgent).mockImplementationOnce(() => new Promise(() => undefined) as never); const resultPromise = generatePrMetadata({ task: createTask(), @@ -187,7 +196,32 @@ describe("generatePrMetadata", () => { body: expect.stringContaining("Closes FN-4991"), templateUsed: false, }); - expect(disposeMock).toHaveBeenCalledTimes(1); + const result = await resultPromise; + expectFallbackBody(result.body); + expect(promptMock).not.toHaveBeenCalled(); + expect(disposeMock).not.toHaveBeenCalled(); + }); + + it("returns fallback when prompt times out", async () => { + vi.useFakeTimers(); + promptMock.mockImplementation(() => new Promise(() => undefined)); + + const resultPromise = generatePrMetadata({ + task: createTask(), + repoRoot, + settings: {} as never, + timeoutMs: 1_000, + }); + + await vi.advanceTimersByTimeAsync(1_000); + + await expect(resultPromise).resolves.toEqual({ + title: "Route contracts", + body: expect.stringContaining("Closes FN-4991"), + templateUsed: false, + }); + const result = await resultPromise; + expectFallbackBody(result.body); }); it("returns fallback immediately when caller signal is already aborted", async () => { @@ -210,15 +244,9 @@ describe("generatePrMetadata", () => { expect(promptMock).not.toHaveBeenCalled(); }); - it("returns fallback and disposes the session when caller aborts mid-generation", async () => { + it("returns fallback when caller aborts mid-generation", async () => { const controller = new AbortController(); - let promptStarted!: () => void; - const promptStartedPromise = new Promise((resolve) => { - promptStarted = resolve; - }); - promptMock.mockImplementation(() => new Promise(() => { - promptStarted(); - })); + vi.mocked(createFnAgent).mockImplementationOnce(() => new Promise(() => undefined) as never); const resultPromise = generatePrMetadata({ task: createTask(), @@ -227,7 +255,6 @@ describe("generatePrMetadata", () => { signal: controller.signal, }); - await promptStartedPromise; controller.abort(); await expect(resultPromise).resolves.toEqual({ @@ -235,7 +262,7 @@ describe("generatePrMetadata", () => { body: expect.stringContaining("Closes FN-4991"), templateUsed: false, }); - expect(promptMock).toHaveBeenCalledWith(expect.any(String), expect.objectContaining({ signal: expect.any(AbortSignal) })); - expect(disposeMock).toHaveBeenCalledTimes(1); + const result = await resultPromise; + expectFallbackBody(result.body); }); }); diff --git a/packages/dashboard/src/__tests__/pr-routes.contract.test.ts b/packages/dashboard/src/__tests__/pr-routes.contract.test.ts index 35425e7f79..b833b33571 100644 --- a/packages/dashboard/src/__tests__/pr-routes.contract.test.ts +++ b/packages/dashboard/src/__tests__/pr-routes.contract.test.ts @@ -85,6 +85,16 @@ describe("PR routes contract", () => { expect(response.body.error).toContain("title is required"); }); + it("rejects PR create requests with a title but no body before GitHub invocation", async () => { + const app = createServer(createStore(createTask({ prInfo: undefined }))); + const response = await performRequest(app, "POST", "/api/tasks/FN-001/pr/create", JSON.stringify({ title: "Manual title", body: " " }), { + "content-type": "application/json", + }); + + expect(response.status).toBe(400); + expect(response.body.error).toContain("body is required"); + }); + it("does not return conflict when task already has PR", async () => { const app = createServer(createStore(createTask())); const response = await performRequest(app, "POST", "/api/tasks/FN-001/pr/create", JSON.stringify({ title: "x" }), { diff --git a/packages/dashboard/src/__tests__/register-git-github.pr-errors.test.ts b/packages/dashboard/src/__tests__/register-git-github.pr-errors.test.ts index 89b6333b96..efa413ded9 100644 --- a/packages/dashboard/src/__tests__/register-git-github.pr-errors.test.ts +++ b/packages/dashboard/src/__tests__/register-git-github.pr-errors.test.ts @@ -80,7 +80,7 @@ describe("PR route structured GitHub errors", () => { vi.spyOn(GitHubClient.prototype, "findPrForBranch").mockRejectedValue(new Error("authentication required 401")); const app = createServer(createStore(task)); - const response = await performRequest(app, "POST", "/api/tasks/FN-001/pr/create", JSON.stringify({ title: "PR title" }), { "content-type": "application/json" }); + const response = await performRequest(app, "POST", "/api/tasks/FN-001/pr/create", JSON.stringify({ title: "PR title", body: "PR body" }), { "content-type": "application/json" }); expect(response.status).toBe(401); expect(response.body.details.githubError.code).toBe("not-authenticated"); diff --git a/packages/dashboard/src/__tests__/register-git-github.pr-options-preflight-metadata.test.ts b/packages/dashboard/src/__tests__/register-git-github.pr-options-preflight-metadata.test.ts index c41fe2e77e..d0fbd0ab85 100644 --- a/packages/dashboard/src/__tests__/register-git-github.pr-options-preflight-metadata.test.ts +++ b/packages/dashboard/src/__tests__/register-git-github.pr-options-preflight-metadata.test.ts @@ -10,6 +10,11 @@ const { mockGeneratePrMetadata } = vi.hoisted(() => ({ vi.mock("../pr-metadata-generator.js", () => ({ generatePrMetadata: mockGeneratePrMetadata, + buildFallbackPrMetadata: (task: Task) => ({ + title: task.title ?? task.id, + body: ["## Summary", "", task.description ?? "Summary unavailable.", "", "## Changes", "", "- Details unavailable.", "", "## Testing", "", "- Not provided.", "", "## Linked Task", "", `Closes ${task.id}`].join("\n"), + templateUsed: false, + }), })); import { prRouteCommandRunner } from "../routes/register-git-github.js"; @@ -126,6 +131,7 @@ describe("PR metadata/preflight/options routes", () => { }); afterEach(() => { + vi.useRealTimers(); vi.restoreAllMocks(); if (originalRepoEnv === undefined) { delete process.env.GITHUB_REPOSITORY; @@ -144,9 +150,28 @@ describe("PR metadata/preflight/options routes", () => { task: expect.objectContaining({ id: "FN-001" }), repoRoot: "/tmp/project", signal: expect.any(AbortSignal), + timeoutMs: 25_000, })); }); + it("POST /pr/generate-metadata returns deterministic fallback when the generator never resolves", async () => { + vi.useFakeTimers(); + mockGeneratePrMetadata.mockImplementationOnce(() => new Promise(() => undefined)); + const app = createServer(createStore(createTask())); + + const responsePromise = performRequest(app, "POST", "/api/tasks/FN-001/pr/generate-metadata", "{}", { "content-type": "application/json" }); + await vi.advanceTimersByTimeAsync(25_000); + const response = await responsePromise; + + expect(response.status).toBe(200); + expect(response.body).toMatchObject({ title: "Task", templateUsed: false }); + expect(response.body.body).toContain("## Summary"); + expect(response.body.body).toContain("## Changes"); + expect(response.body.body).toContain("## Testing"); + expect(response.body.body).toContain("## Linked Task"); + expect(response.body.body).toContain("Closes FN-001"); + }); + it("POST /pr/generate-metadata returns 404 for missing task", async () => { const missing = Object.assign(new Error("missing"), { code: "ENOENT" }); const app = createServer(createStore(missing)); diff --git a/packages/dashboard/src/__tests__/routes-auth.test.ts b/packages/dashboard/src/__tests__/routes-auth.test.ts index 48d04cfae6..e7bee8f53b 100644 --- a/packages/dashboard/src/__tests__/routes-auth.test.ts +++ b/packages/dashboard/src/__tests__/routes-auth.test.ts @@ -3058,7 +3058,7 @@ describe("Pause/Unpause endpoints", () => { buildApp(), "POST", "/api/tasks/KB-001/pr/create", - JSON.stringify({ title: "Test PR" }), + JSON.stringify({ title: "Test PR", body: "Test body" }), { "Content-Type": "application/json" } ); @@ -3081,7 +3081,7 @@ describe("Pause/Unpause endpoints", () => { buildApp(), "POST", "/api/tasks/KB-001/pr/create", - JSON.stringify({ title: "Test PR" }), + JSON.stringify({ title: "Test PR", body: "Test body" }), { "Content-Type": "application/json" } ); @@ -3138,7 +3138,7 @@ describe("Pause/Unpause endpoints", () => { app, "POST", `/api/tasks/KB-RATE-${i}/pr/create`, - JSON.stringify({ title: `Test PR ${i}` }), + JSON.stringify({ title: `Test PR ${i}`, body: "Test body" }), { "Content-Type": "application/json" } ); // Should not get 429 from our code (may get 500 from gh CLI not being available in test) @@ -3166,7 +3166,7 @@ describe("Pause/Unpause endpoints", () => { buildApp(), "POST", "/api/tasks/KB-001/pr/create", - JSON.stringify({ title: "Test PR" }), + JSON.stringify({ title: "Test PR", body: "Test body" }), { "Content-Type": "application/json" } ); @@ -3192,7 +3192,7 @@ describe("Pause/Unpause endpoints", () => { buildApp(), "POST", "/api/tasks/KB-001/pr/create", - JSON.stringify({ title: "Test PR" }), + JSON.stringify({ title: "Test PR", body: "Test body" }), { "Content-Type": "application/json" } ); @@ -3217,7 +3217,7 @@ describe("Pause/Unpause endpoints", () => { buildApp(), "POST", "/api/tasks/KB-999/pr/create", - JSON.stringify({ title: "Test PR" }), + JSON.stringify({ title: "Test PR", body: "Test body" }), { "Content-Type": "application/json" } ); diff --git a/packages/dashboard/src/__tests__/routes-github.test.ts b/packages/dashboard/src/__tests__/routes-github.test.ts index 3d1958923a..b288806ef3 100644 --- a/packages/dashboard/src/__tests__/routes-github.test.ts +++ b/packages/dashboard/src/__tests__/routes-github.test.ts @@ -2557,7 +2557,7 @@ describe("PR conflict refresh + reclaim routes", () => { commentCount: 0, } as any); - const res = await REQUEST(buildApp(), "POST", `/api/tasks/${task.id}/pr/create`, JSON.stringify({ title: "New PR" }), { "content-type": "application/json" }); + const res = await REQUEST(buildApp(), "POST", `/api/tasks/${task.id}/pr/create`, JSON.stringify({ title: "New PR", body: "New PR body" }), { "content-type": "application/json" }); expect(res.status).toBe(201); expect(store.addPrInfo).toHaveBeenCalledWith(task.id, expect.objectContaining({ number: 901 })); diff --git a/packages/dashboard/src/github.ts b/packages/dashboard/src/github.ts index 111b349656..d78d584789 100644 --- a/packages/dashboard/src/github.ts +++ b/packages/dashboard/src/github.ts @@ -153,6 +153,14 @@ export interface CreateIssueParams { labels?: string[]; } +function requireNonEmptyPrBody(body: string | undefined): string { + const trimmed = body?.trim() ?? ""; + if (!trimmed) { + throw new Error("PR body is required when creating a pull request"); + } + return trimmed; +} + export interface CreatedIssue { owner: string; repo: string; @@ -828,6 +836,11 @@ export class GitHubClient { private createPrWithGh(params: CreatePrParams): PrInfo { const { owner: paramOwner, repo: paramRepo, title, body, head, base, draft, reviewers } = params; const { owner, repo } = this.resolveRepo(paramOwner, paramRepo); + /* + FNXC:GitHubPrCreate 2026-06-23-00:00: + The Create PR flow runs `gh pr create` non-interactively, so a title without a body fails at the CLI boundary. Validate here as a final guard in addition to dashboard/API checks and always pass `--body` with non-empty content. + */ + const prBody = requireNonEmptyPrBody(body); // Build gh pr create command arguments (as array for safety) const args = [ @@ -835,11 +848,9 @@ export class GitHubClient { "--repo", `${owner}/${repo}`, "--title", title, "--head", head, + "--body", prBody, ]; - if (body) { - args.push("--body", body); - } if (base) { args.push("--base", base); } @@ -877,6 +888,7 @@ export class GitHubClient { private async createPrWithApi(params: CreatePrParams): Promise { const { owner: paramOwner, repo: paramRepo, title, body, head, base = "main", draft, reviewers } = params; const { owner, repo } = this.resolveRepo(paramOwner, paramRepo); + const prBody = requireNonEmptyPrBody(body); const url = `${this.baseUrl}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/pulls`; @@ -887,7 +899,7 @@ export class GitHubClient { headers, body: JSON.stringify({ title, - body: body || "", + body: prBody, head, base, draft: draft === true, diff --git a/packages/dashboard/src/pr-metadata-generator.ts b/packages/dashboard/src/pr-metadata-generator.ts index dbe994b3e2..978872b52d 100644 --- a/packages/dashboard/src/pr-metadata-generator.ts +++ b/packages/dashboard/src/pr-metadata-generator.ts @@ -7,7 +7,7 @@ import { resolveTitleSummarizerSettingsModel } from "@fusion/core"; import { createFnAgent } from "@fusion/engine"; const execAsync = promisify(execCb); -const PR_METADATA_TIMEOUT_MS = 60_000; +export const PR_METADATA_TIMEOUT_MS = 60_000; export interface GeneratedPrMetadata { title: string; @@ -23,7 +23,7 @@ interface AiMetadataResult { linkedTask: string; } -function buildFallback(task: Task): GeneratedPrMetadata { +export function buildFallbackPrMetadata(task: Task): GeneratedPrMetadata { return { title: task.title ?? task.id, body: [ @@ -136,12 +136,13 @@ function fillTemplate(template: string, result: AiMetadataResult, taskId: string } async function runCommand(command: string, cwd: string, signal?: AbortSignal): Promise { - const { stdout } = await execAsync(command, { + const commandPromise = execAsync(command, { cwd, timeout: 15_000, maxBuffer: 10 * 1024 * 1024, signal, }); + const { stdout } = signal ? await raceWithAbort(commandPromise, signal) : await commandPromise; return stdout.trim(); } @@ -158,20 +159,45 @@ function throwIfAborted(signal?: AbortSignal): void { throw signal.reason instanceof Error ? signal.reason : createAbortError(); } -function waitForAbort(signal: AbortSignal): Promise { - return new Promise((_, reject) => { - if (signal.aborted) { - reject(signal.reason instanceof Error ? signal.reason : createAbortError()); - return; - } - signal.addEventListener( - "abort", - () => reject(signal.reason instanceof Error ? signal.reason : createAbortError()), - { once: true }, +function getAbortReason(signal: AbortSignal): Error { + return signal.reason instanceof Error ? signal.reason : createAbortError(); +} + +function raceWithAbort(operation: PromiseLike, signal: AbortSignal): Promise { + if (signal.aborted) { + return Promise.reject(getAbortReason(signal)); + } + return new Promise((resolve, reject) => { + const cleanup = () => signal.removeEventListener("abort", onAbort); + const onAbort = () => { + cleanup(); + reject(getAbortReason(signal)); + }; + signal.addEventListener("abort", onAbort, { once: true }); + Promise.resolve(operation).then( + (value) => { + cleanup(); + resolve(value); + }, + (error) => { + cleanup(); + reject(error); + }, ); }); } +function disposeSessionBestEffort(session: { dispose?: () => void | Promise }): void { + try { + const disposeResult = session.dispose?.(); + if (disposeResult && typeof (disposeResult as Promise).catch === "function") { + void (disposeResult as Promise).catch(() => undefined); + } + } catch { + // best effort + } +} + function isAbortLikeError(error: unknown): boolean { return Boolean( error @@ -202,9 +228,13 @@ export async function generatePrMetadata(input: { timeoutMs?: number; }): Promise { const { task, repoRoot, settings, signal, timeoutMs = PR_METADATA_TIMEOUT_MS } = input; - const fallback = buildFallback(task); + const fallback = buildFallbackPrMetadata(task); const controller = new AbortController(); const abortFromCaller = () => controller.abort(signal?.reason instanceof Error ? signal.reason : createAbortError()); + /* + FNXC:PrMetadataGeneration 2026-06-23-00:00: + The Create PR dialog must receive usable title/body metadata within its UX budget even when git, template reads, provider startup, prompt streaming, or disposal misbehaves. Race every awaited operation against one shared abort signal so provider/session hangs degrade to deterministic fallback content instead of leaving the modal spinner pending indefinitely. + */ const timeoutId = setTimeout(() => controller.abort(createAbortError()), timeoutMs); const combinedSignal = controller.signal; const callerSignalWasActive = Boolean(signal && !signal.aborted); @@ -220,27 +250,27 @@ export async function generatePrMetadata(input: { try { throwIfAborted(combinedSignal); - const baseBranch = await resolveBaseBranch(task, repoRoot, combinedSignal); - const [logOut, diffStatOut] = await Promise.all([ + const baseBranch = await raceWithAbort(resolveBaseBranch(task, repoRoot, combinedSignal), combinedSignal); + const [logOut, diffStatOut] = await raceWithAbort(Promise.all([ runCommand(`git log --no-merges ${baseBranch}..HEAD --format=%s%n%b`, repoRoot, combinedSignal).catch(() => ""), runCommand(`git diff --stat ${baseBranch}..HEAD`, repoRoot, combinedSignal).catch(() => ""), - ]); + ]), combinedSignal); let promptContent = ""; try { const promptPath = join(repoRoot, ".fusion", "tasks", task.id, "PROMPT.md"); - promptContent = (await readFile(promptPath, "utf8")).trim(); + promptContent = (await raceWithAbort(readFile(promptPath, "utf8"), combinedSignal)).trim(); } catch { promptContent = ""; } const templatePath = join(repoRoot, ".github", "pull_request_template.md"); - const templateExists = await access(templatePath).then(() => true).catch(() => false); - const template = templateExists ? await readFile(templatePath, "utf8") : ""; + const templateExists = await raceWithAbort(access(templatePath).then(() => true).catch(() => false), combinedSignal); + const template = templateExists ? await raceWithAbort(readFile(templatePath, "utf8"), combinedSignal) : ""; const model = resolveTitleSummarizerSettingsModel(settings as Partial); let aiText = ""; - const { session } = await createFnAgent({ + const { session } = await raceWithAbort(createFnAgent({ cwd: repoRoot, tools: "readonly", defaultProvider: model.provider, @@ -253,7 +283,7 @@ export async function generatePrMetadata(input: { onText: (delta: string) => { aiText += delta; }, - }); + }), combinedSignal); try { const contextPrompt = [ @@ -270,16 +300,12 @@ export async function generatePrMetadata(input: { ].join("\n\n"); throwIfAborted(combinedSignal); - await Promise.race([ + await raceWithAbort( (session.prompt as (prompt: string, options?: { signal?: AbortSignal }) => Promise)(contextPrompt, { signal: combinedSignal }), - waitForAbort(combinedSignal), - ]); + combinedSignal, + ); } finally { - try { - session.dispose(); - } catch { - // best effort - } + disposeSessionBestEffort(session); } const parsed = parseAiResult(aiText); diff --git a/packages/dashboard/src/routes/register-git-github.ts b/packages/dashboard/src/routes/register-git-github.ts index 0e08a14aa3..479826ae9d 100644 --- a/packages/dashboard/src/routes/register-git-github.ts +++ b/packages/dashboard/src/routes/register-git-github.ts @@ -46,7 +46,7 @@ import { GitHubSourceIssueCloseService } from "../github-source-issue-close.js"; import { KnowledgeIndexRefreshService } from "../knowledge-index-refresh.js"; import { githubRateLimiter } from "../github-poll.js"; import * as projectStoreResolver from "../project-store-resolver.js"; -import { generatePrMetadata } from "../pr-metadata-generator.js"; +import { buildFallbackPrMetadata, generatePrMetadata } from "../pr-metadata-generator.js"; import { resolvePrConflicts } from "../pr-conflict-resolver.js"; import { classifyWebhookEvent, @@ -62,6 +62,7 @@ const execAsync = promisify(execCb); const PR_ROUTE_MAX_BUFFER_BYTES = 10 * 1024 * 1024; const PR_PREFLIGHT_TIMEOUT_MS = 15_000; const PR_OPTIONS_TIMEOUT_MS = 10_000; +const PR_METADATA_ROUTE_TIMEOUT_MS = 25_000; const SAFE_GIT_REF_PATTERN = /^[A-Za-z0-9._/-]+$/; export const GITHUB_TRACKING_RECONCILE_INTERVAL_MS = 15 * 60 * 1000; @@ -4855,7 +4856,7 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { /** * POST /api/tasks/:id/pr/create * Create a GitHub PR for an in-review task. - * Body: { title: string, body?: string, base?: string } + * Body: { title: string, body: string, base?: string } * Returns: Created PrInfo */ router.post("/tasks/:id/pr/create", async (req, res) => { @@ -4863,16 +4864,21 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { const { store: scopedStore } = await getProjectContext(req); const { title, body, base } = req.body; - if (!title || typeof title !== "string") { - throw badRequest("title is required and must be a string"); - } - // Get task and validate const task = await scopedStore.getTask(req.params.id); if (task.column !== "in-review") { throw badRequest("Task must be in 'in-review' column to create a PR"); } + if (!title || typeof title !== "string") { + throw badRequest("title is required and must be a string"); + } + if (!body || typeof body !== "string" || !body.trim()) { + throw badRequest("body is required and must be a non-empty string"); + } + const prTitle = title.trim(); + const prBody = body.trim(); + const existingPrs = getTaskPrList(task); // Determine branch name from task @@ -4920,8 +4926,8 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { prInfo = await client.createPr({ owner, repo, - title, - body, + title: prTitle, + body: prBody, head: branchName, base, }); @@ -5098,20 +5104,53 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { */ router.post("/tasks/:id/pr/generate-metadata", async (req, res) => { const controller = new AbortController(); - const abortRequest = () => controller.abort(); - req.on("close", abortRequest); + let responseCompleted = false; + const abortRequest = () => { + if (!responseCompleted && !controller.signal.aborted) { + const error = new Error("PR metadata request aborted"); + error.name = "AbortError"; + controller.abort(error); + } + }; + const markCompleted = () => { + responseCompleted = true; + }; + req.on("aborted", abortRequest); + res.on("close", abortRequest); + res.on("finish", markCompleted); try { const { store: scopedStore } = await getProjectContext(req); const task = await scopedStore.getTask(req.params.id); const settings = await scopedStore.getSettings(); - const metadata = await generatePrMetadata({ + /* + FNXC:PrMetadataGeneration 2026-06-23-00:00: + The dashboard route owns a shorter Create PR dialog budget than the reusable generator default. If a mocked or broken generator ignores aborts and never settles, return deterministic fallback metadata before the modal crosses the 30s UX threshold; do not treat the timeout abort as a disconnected-client signal. + */ + let timeoutId: ReturnType | undefined; + const generatorPromise = generatePrMetadata({ task, repoRoot: scopedStore.getRootDir(), settings, signal: controller.signal, + timeoutMs: PR_METADATA_ROUTE_TIMEOUT_MS, }); - if (req.destroyed || res.writableEnded || res.writableFinished || controller.signal.aborted) { + void generatorPromise.catch(() => undefined); + const timeoutFallbackPromise = new Promise>>((resolve) => { + timeoutId = setTimeout(() => { + abortRequest(); + resolve(buildFallbackPrMetadata(task)); + }, PR_METADATA_ROUTE_TIMEOUT_MS); + }); + let metadata: Awaited>; + try { + metadata = await Promise.race([generatorPromise, timeoutFallbackPromise]); + } finally { + if (timeoutId) { + clearTimeout(timeoutId); + } + } + if (res.writableEnded || res.writableFinished || res.destroyed) { return; } res.json(metadata); @@ -5124,7 +5163,9 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void { } rethrowAsApiError(err, "Failed to generate PR metadata"); } finally { - req.off("close", abortRequest); + req.removeListener("aborted", abortRequest); + res.removeListener("close", abortRequest); + res.removeListener("finish", markCompleted); } });