diff --git a/lib/coding-agent/__tests__/handlePRCreated.test.ts b/lib/coding-agent/__tests__/handlePRCreated.test.ts index 09444996..3ccac045 100644 --- a/lib/coding-agent/__tests__/handlePRCreated.test.ts +++ b/lib/coding-agent/__tests__/handlePRCreated.test.ts @@ -23,7 +23,7 @@ vi.mock("../prState", () => ({ })); describe("handlePRCreated", () => { - it("posts a card with PR links and merge button", async () => { + it("posts a card with PR links and individual merge buttons", async () => { const { handlePRCreated } = await import("../handlePRCreated"); await handlePRCreated("slack:C123:ts", { @@ -47,7 +47,10 @@ describe("handlePRCreated", () => { const { Button } = await import("chat"); expect(Button).toHaveBeenCalledWith( - expect.objectContaining({ id: "merge_all_prs", label: "Merge All PRs" }), + expect.objectContaining({ + id: "merge_pr:recoupable/api#42", + label: "Merge recoupable/api#42", + }), ); expect(mockThread.setState).toHaveBeenCalledWith( diff --git a/lib/coding-agent/__tests__/mergeGithubPR.test.ts b/lib/coding-agent/__tests__/mergeGithubPR.test.ts new file mode 100644 index 00000000..889f23f6 --- /dev/null +++ b/lib/coding-agent/__tests__/mergeGithubPR.test.ts @@ -0,0 +1,39 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { mergeGithubPR } from "../mergeGithubPR"; + +global.fetch = vi.fn(); + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe("mergeGithubPR", () => { + it("squash-merges a PR and returns ok", async () => { + vi.mocked(fetch).mockResolvedValue({ ok: true } as Response); + + const result = await mergeGithubPR("recoupable/api", 42, "ghp_test"); + + expect(result).toEqual({ ok: true }); + expect(fetch).toHaveBeenCalledWith( + "https://api.github.com/repos/recoupable/api/pulls/42/merge", + expect.objectContaining({ + method: "PUT", + body: JSON.stringify({ merge_method: "squash" }), + }), + ); + }); + + it("returns error message on failure", async () => { + const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + vi.mocked(fetch).mockResolvedValue({ + ok: false, + status: 405, + text: () => Promise.resolve(JSON.stringify({ message: "Not allowed" })), + } as any); + + const result = await mergeGithubPR("recoupable/api", 42, "ghp_test"); + + expect(result).toEqual({ ok: false, message: "Not allowed" }); + consoleSpy.mockRestore(); + }); +}); diff --git a/lib/coding-agent/__tests__/onMergeAction.test.ts b/lib/coding-agent/__tests__/onMergeAction.test.ts index a67d31e6..fde84379 100644 --- a/lib/coding-agent/__tests__/onMergeAction.test.ts +++ b/lib/coding-agent/__tests__/onMergeAction.test.ts @@ -1,18 +1,32 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; -global.fetch = vi.fn(); +const mockMergeGithubPR = vi.fn(); +vi.mock("../mergeGithubPR", () => ({ + mergeGithubPR: (...args: unknown[]) => mockMergeGithubPR(...args), +})); const mockHandleMergeSuccess = vi.fn(); vi.mock("../handleMergeSuccess", () => ({ handleMergeSuccess: (...args: unknown[]) => mockHandleMergeSuccess(...args), })); +const mockDeletePRState = vi.fn(); +vi.mock("../prState", () => ({ + deleteCodingAgentPRState: (...args: unknown[]) => mockDeletePRState(...args), +})); + +const mockUpsertAccountSnapshot = vi.fn(); +vi.mock("@/lib/supabase/account_snapshots/upsertAccountSnapshot", () => ({ + upsertAccountSnapshot: (...args: unknown[]) => mockUpsertAccountSnapshot(...args), +})); + const { registerOnMergeAction } = await import("../handlers/onMergeAction"); beforeEach(() => { vi.clearAllMocks(); process.env.GITHUB_TOKEN = "ghp_test"; mockHandleMergeSuccess.mockResolvedValue(undefined); + mockUpsertAccountSnapshot.mockResolvedValue({ data: {}, error: null }); }); /** @@ -25,18 +39,34 @@ function createMockBot() { } describe("registerOnMergeAction", () => { - it("registers merge_all_prs action handler", () => { + it("registers catch-all action handler", () => { const bot = createMockBot(); registerOnMergeAction(bot); - expect(bot.onAction).toHaveBeenCalledWith("merge_all_prs", expect.any(Function)); + expect(bot.onAction).toHaveBeenCalledWith(expect.any(Function)); }); - it("squash-merges PRs, calls handleMergeSuccess, and posts results", async () => { - vi.mocked(fetch).mockResolvedValue({ ok: true } as Response); + it("ignores non-merge actions", async () => { + const bot = createMockBot(); + registerOnMergeAction(bot); + const handler = bot.onAction.mock.calls[0][0]; + + const mockThread = { + state: Promise.resolve({ status: "pr_created", prompt: "fix bug", prs: [] }), + post: vi.fn(), + setState: vi.fn(), + }; + + await handler({ thread: mockThread, actionId: "some_other_action" }); + + expect(mockThread.post).not.toHaveBeenCalled(); + }); + + it("squash-merges a single PR, calls handleMergeSuccess, and posts result", async () => { + mockMergeGithubPR.mockResolvedValue({ ok: true }); const bot = createMockBot(); registerOnMergeAction(bot); - const handler = bot.onAction.mock.calls[0][1]; + const handler = bot.onAction.mock.calls[0][0]; const mockThread = { state: Promise.resolve({ @@ -50,66 +80,125 @@ describe("registerOnMergeAction", () => { setState: vi.fn(), }; - await handler({ thread: mockThread }); + await handler({ thread: mockThread, actionId: "merge_pr:recoupable/api#42" }); - expect(fetch).toHaveBeenCalledWith( - "https://api.github.com/repos/recoupable/api/pulls/42/merge", - expect.objectContaining({ method: "PUT" }), - ); - expect(mockThread.setState).toHaveBeenCalledWith({ status: "merged" }); + expect(mockMergeGithubPR).toHaveBeenCalledWith("recoupable/api", 42, "ghp_test"); + expect(mockThread.setState).toHaveBeenCalledWith({ status: "merged", prs: [] }); expect(mockHandleMergeSuccess).toHaveBeenCalledWith( expect.objectContaining({ branch: "agent/fix-bug", snapshotId: "snap_abc123" }), ); - expect(mockThread.post).toHaveBeenCalledWith(expect.stringContaining("merged")); + expect(mockThread.post).toHaveBeenCalledWith("✅ recoupable/api#42 merged."); }); - it("does not call handleMergeSuccess when a merge fails", async () => { - vi.mocked(fetch).mockResolvedValue({ - ok: false, - status: 409, - text: () => Promise.resolve(JSON.stringify({ message: "merge conflict" })), - } as unknown as Response); + it("merges one PR and keeps remaining PRs in state without calling handleMergeSuccess", async () => { + mockMergeGithubPR.mockResolvedValue({ ok: true }); const bot = createMockBot(); registerOnMergeAction(bot); - const handler = bot.onAction.mock.calls[0][1]; - const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const handler = bot.onAction.mock.calls[0][0]; const mockThread = { state: Promise.resolve({ status: "pr_created", prompt: "fix bug", branch: "agent/fix-bug", - snapshotId: "snap_abc123", - prs: [{ repo: "recoupable/api", number: 42, url: "url", baseBranch: "test" }], + prs: [ + { repo: "recoupable/api", number: 42, url: "url1", baseBranch: "test" }, + { repo: "recoupable/chat", number: 10, url: "url2", baseBranch: "test" }, + ], }), post: vi.fn(), setState: vi.fn(), }; - await handler({ thread: mockThread }); + await handler({ thread: mockThread, actionId: "merge_pr:recoupable/api#42" }); + expect(mockThread.setState).toHaveBeenCalledWith({ + status: "pr_created", + prs: [{ repo: "recoupable/chat", number: 10, url: "url2", baseBranch: "test" }], + }); expect(mockHandleMergeSuccess).not.toHaveBeenCalled(); - expect(mockThread.setState).toHaveBeenCalledWith({ status: "pr_created" }); - expect(mockThread.post).toHaveBeenCalledWith(expect.stringContaining("failed")); - consoleSpy.mockRestore(); + expect(mockThread.post).toHaveBeenCalledWith("✅ recoupable/api#42 merged."); }); - it("posts no PRs message when state has no PRs", async () => { + it("posts not found message when PR is not in thread state", async () => { const bot = createMockBot(); registerOnMergeAction(bot); - const handler = bot.onAction.mock.calls[0][1]; + const handler = bot.onAction.mock.calls[0][0]; const mockThread = { - state: Promise.resolve({ status: "pr_created", prompt: "fix bug" }), + state: Promise.resolve({ status: "pr_created", prompt: "fix bug", prs: [] }), post: vi.fn(), setState: vi.fn(), }; - await handler({ thread: mockThread }); + await handler({ thread: mockThread, actionId: "merge_pr:recoupable/api#99" }); - expect(mockThread.post).toHaveBeenCalledWith("No PRs to merge."); - expect(fetch).not.toHaveBeenCalled(); + expect(mockThread.post).toHaveBeenCalledWith("PR recoupable/api#99 not found in this thread."); + expect(mockMergeGithubPR).not.toHaveBeenCalled(); expect(mockHandleMergeSuccess).not.toHaveBeenCalled(); }); + + it("posts error message when merge fails", async () => { + mockMergeGithubPR.mockResolvedValue({ ok: false, message: "Not allowed" }); + + const bot = createMockBot(); + registerOnMergeAction(bot); + const handler = bot.onAction.mock.calls[0][0]; + + const mockThread = { + state: Promise.resolve({ + status: "pr_created", + prompt: "fix bug", + branch: "agent/fix-bug", + prs: [{ repo: "recoupable/api", number: 42, url: "url", baseBranch: "test" }], + }), + post: vi.fn(), + setState: vi.fn(), + }; + + await handler({ thread: mockThread, actionId: "merge_pr:recoupable/api#42" }); + + expect(mockHandleMergeSuccess).not.toHaveBeenCalled(); + expect(mockThread.post).toHaveBeenCalledWith("❌ recoupable/api#42 failed to merge: Not allowed"); + }); + + it("merge button click triggers snapshot upsert in Supabase via handleMergeSuccess", async () => { + // Use the real handleMergeSuccess instead of the mock to verify the full chain + const { handleMergeSuccess: realHandleMergeSuccess } = await vi.importActual< + typeof import("../handleMergeSuccess") + >("../handleMergeSuccess"); + mockHandleMergeSuccess.mockImplementation(realHandleMergeSuccess); + mockMergeGithubPR.mockResolvedValue({ ok: true }); + + const bot = createMockBot(); + registerOnMergeAction(bot); + const handler = bot.onAction.mock.calls[0][0]; + + const state = { + status: "pr_created" as const, + prompt: "fix bug", + branch: "agent/fix-bug", + snapshotId: "snap_abc123", + prs: [{ repo: "recoupable/api", number: 42, url: "url", baseBranch: "test" }], + }; + + const mockThread = { + state: Promise.resolve(state), + post: vi.fn(), + setState: vi.fn(), + }; + + await handler({ thread: mockThread, actionId: "merge_pr:recoupable/api#42" }); + + // Verify the full chain: merge → handleMergeSuccess → upsertAccountSnapshot + expect(mockMergeGithubPR).toHaveBeenCalledWith("recoupable/api", 42, "ghp_test"); + expect(mockDeletePRState).toHaveBeenCalledWith("recoupable/api", "agent/fix-bug"); + expect(mockUpsertAccountSnapshot).toHaveBeenCalledWith( + expect.objectContaining({ + snapshot_id: "snap_abc123", + expires_at: expect.any(String), + }), + ); + }); }); diff --git a/lib/coding-agent/__tests__/parseMergeActionId.test.ts b/lib/coding-agent/__tests__/parseMergeActionId.test.ts new file mode 100644 index 00000000..b4bbb929 --- /dev/null +++ b/lib/coding-agent/__tests__/parseMergeActionId.test.ts @@ -0,0 +1,25 @@ +import { describe, it, expect } from "vitest"; +import { parseMergeActionId } from "../parseMergeActionId"; + +describe("parseMergeActionId", () => { + it("parses a valid merge action ID", () => { + expect(parseMergeActionId("merge_pr:recoupable/api#42")).toEqual({ + repo: "recoupable/api", + number: 42, + }); + }); + + it("parses action ID with nested org/repo", () => { + expect(parseMergeActionId("merge_pr:org/sub-repo#7")).toEqual({ + repo: "org/sub-repo", + number: 7, + }); + }); + + it("returns null for invalid format", () => { + expect(parseMergeActionId("merge_all_prs")).toBeNull(); + expect(parseMergeActionId("merge_pr:")).toBeNull(); + expect(parseMergeActionId("merge_pr:repo")).toBeNull(); + expect(parseMergeActionId("other_action:repo#1")).toBeNull(); + }); +}); diff --git a/lib/coding-agent/buildPRCard.ts b/lib/coding-agent/buildPRCard.ts index 9ca6343d..b46c6c1f 100644 --- a/lib/coding-agent/buildPRCard.ts +++ b/lib/coding-agent/buildPRCard.ts @@ -2,7 +2,7 @@ import { Card, CardText, Actions, Button, LinkButton } from "chat"; import type { CodingAgentPR } from "./types"; /** - * Builds a Card with PR review links and a Merge All PRs button. + * Builds a Card with PR review links and individual Merge buttons per PR. * * @param title - Card title (e.g. "PRs Created", "PRs Updated") * @param prs - Array of PRs to build review links for @@ -13,10 +13,14 @@ export function buildPRCard(title: string, prs: CodingAgentPR[]) { children: [ CardText(`${prs.map(pr => `- ${pr.repo}#${pr.number} → \`${pr.baseBranch}\``).join("\n")}\n\nReply in this thread to give feedback.`), Actions([ - ...prs.map(pr => + ...prs.flatMap(pr => [ LinkButton({ url: pr.url, label: `Review ${pr.repo}#${pr.number}` }), - ), - Button({ id: "merge_all_prs", label: "Merge All PRs", style: "primary" }), + Button({ + id: `merge_pr:${pr.repo}#${pr.number}`, + label: `Merge ${pr.repo}#${pr.number}`, + style: "primary", + }), + ]), ]), ], }); diff --git a/lib/coding-agent/handlers/onMergeAction.ts b/lib/coding-agent/handlers/onMergeAction.ts index 3283d2c9..027a6e0a 100644 --- a/lib/coding-agent/handlers/onMergeAction.ts +++ b/lib/coding-agent/handlers/onMergeAction.ts @@ -1,21 +1,28 @@ import type { CodingAgentBot } from "../bot"; import type { CodingAgentThreadState } from "../types"; import { handleMergeSuccess } from "../handleMergeSuccess"; +import { parseMergeActionId } from "../parseMergeActionId"; +import { mergeGithubPR } from "../mergeGithubPR"; /** - * Registers the "Merge All PRs" button action handler on the bot. - * Squash-merges each PR via the GitHub API, then delegates to - * handleMergeSuccess to clean up PR state and persist the latest snapshot. + * Registers individual per-PR merge button action handlers on the bot. + * Each button has an ID like "merge_pr:#" and squash-merges + * that single PR via the GitHub API. + * + * Uses a prefix pattern so a single handler covers all merge_pr:* actions. * * @param bot */ export function registerOnMergeAction(bot: CodingAgentBot) { - bot.onAction("merge_all_prs", async event => { + bot.onAction(async event => { + if (!event.actionId.startsWith("merge_pr:")) return; + const thread = event.thread; const state = (await thread.state) as CodingAgentThreadState | null; - if (!state?.prs?.length) { - await thread.post("No PRs to merge."); + const parsed = parseMergeActionId(event.actionId); + if (!parsed) { + await thread.post("Invalid merge action."); return; } @@ -25,41 +32,38 @@ export function registerOnMergeAction(bot: CodingAgentBot) { return; } - const results: string[] = []; + const pr = state?.prs?.find( + p => p.repo === parsed.repo && p.number === parsed.number, + ); - for (const pr of state.prs) { - const [owner, repo] = pr.repo.split("/"); - const response = await fetch( - `https://api.github.com/repos/${owner}/${repo}/pulls/${pr.number}/merge`, - { - method: "PUT", - headers: { - Authorization: `Bearer ${token}`, - Accept: "application/vnd.github+json", - "X-GitHub-Api-Version": "2022-11-28", - }, - body: JSON.stringify({ merge_method: "squash" }), - }, - ); + if (!pr) { + await thread.post(`PR ${parsed.repo}#${parsed.number} not found in this thread.`); + return; + } - if (response.ok) { - results.push(`${pr.repo}#${pr.number} merged`); - } else { - const errorBody = await response.text(); - console.error(`[coding-agent] merge failed for ${pr.repo}#${pr.number}: ${response.status} ${errorBody}`); - const error = JSON.parse(errorBody); - results.push(`${pr.repo}#${pr.number} failed: ${error.message}`); - } + const result = await mergeGithubPR(pr.repo, pr.number, token); + + if (result.ok === false) { + const { message } = result; + await thread.post(`❌ ${pr.repo}#${pr.number} failed to merge: ${message}`); + return; } - const allMerged = results.every(r => r.endsWith("merged")); + // Remove merged PR from state + const remainingPrs = state!.prs!.filter( + p => !(p.repo === pr.repo && p.number === pr.number), + ); + const allMerged = remainingPrs.length === 0; + + await thread.setState({ + status: allMerged ? "merged" : state!.status, + prs: remainingPrs, + }); - // On failure, revert to pr_created so handleFeedback still accepts replies - await thread.setState({ status: allMerged ? "merged" : "pr_created" }); if (allMerged) { - await handleMergeSuccess(state); + await handleMergeSuccess(state!); } - await thread.post(`Merge results:\n${results.map(r => `- ${r}`).join("\n")}`); + await thread.post(`✅ ${pr.repo}#${pr.number} merged.`); }); } diff --git a/lib/coding-agent/mergeGithubPR.ts b/lib/coding-agent/mergeGithubPR.ts new file mode 100644 index 00000000..cfe70fce --- /dev/null +++ b/lib/coding-agent/mergeGithubPR.ts @@ -0,0 +1,48 @@ +export interface MergeGithubPRSuccess { + ok: true; +} + +export interface MergeGithubPRFailure { + ok: false; + message: string; +} + +export type MergeGithubPRResult = MergeGithubPRSuccess | MergeGithubPRFailure; + +/** + * Squash-merges a GitHub pull request via the API. + * + * @param repo - Full repo identifier (e.g. "recoupable/api") + * @param prNumber - PR number to merge + * @param token - GitHub API token + */ +export async function mergeGithubPR( + repo: string, + prNumber: number, + token: string, +): Promise { + const [owner, repoName] = repo.split("/"); + const response = await fetch( + `https://api.github.com/repos/${owner}/${repoName}/pulls/${prNumber}/merge`, + { + method: "PUT", + headers: { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }, + body: JSON.stringify({ merge_method: "squash" }), + }, + ); + + if (response.ok) { + return { ok: true }; + } + + const errorBody = await response.text(); + console.error( + `[coding-agent] merge failed for ${repo}#${prNumber}: ${response.status} ${errorBody}`, + ); + const error = JSON.parse(errorBody); + return { ok: false, message: error.message }; +} diff --git a/lib/coding-agent/parseMergeActionId.ts b/lib/coding-agent/parseMergeActionId.ts new file mode 100644 index 00000000..5118249e --- /dev/null +++ b/lib/coding-agent/parseMergeActionId.ts @@ -0,0 +1,9 @@ +/** + * Parses a merge action ID like "merge_pr:recoupable/api#42" + * into { repo, number } or null if the format doesn't match. + */ +export function parseMergeActionId(actionId: string) { + const match = actionId.match(/^merge_pr:(.+)#(\d+)$/); + if (!match) return null; + return { repo: match[1], number: parseInt(match[2], 10) }; +}