Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions lib/coding-agent/__tests__/mergeGithubBranch.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { describe, it, expect, vi, beforeEach } from "vitest";
import { mergeGithubBranch } from "../mergeGithubBranch";

global.fetch = vi.fn();

beforeEach(() => {
vi.clearAllMocks();
});

describe("mergeGithubBranch", () => {
it("merges a branch and returns ok", async () => {
vi.mocked(fetch).mockResolvedValue({ ok: true, status: 201 } as Response);

const result = await mergeGithubBranch("recoupable/api", "test", "main", "ghp_test");

expect(result).toEqual({ ok: true });
expect(fetch).toHaveBeenCalledWith(
"https://api.github.com/repos/recoupable/api/merges",
expect.objectContaining({
method: "POST",
body: JSON.stringify({
base: "main",
head: "test",
commit_message: "Merge test into main",
}),
}),
);
});

it("returns ok when already up to date (204)", async () => {
vi.mocked(fetch).mockResolvedValue({ ok: true, status: 204 } as Response);

const result = await mergeGithubBranch("recoupable/chat", "test", "main", "ghp_test");

expect(result).toEqual({ ok: true });
});

it("returns error message on failure", async () => {
const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {});
vi.mocked(fetch).mockResolvedValue({
ok: false,
status: 409,
text: () => Promise.resolve(JSON.stringify({ message: "Merge conflict" })),
} as any);

const result = await mergeGithubBranch("recoupable/api", "test", "main", "ghp_test");

expect(result).toEqual({ ok: false, message: "Merge conflict" });
consoleSpy.mockRestore();
});
});
73 changes: 73 additions & 0 deletions lib/coding-agent/__tests__/onMergeTestToMainAction.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { describe, it, expect, vi, beforeEach } from "vitest";

const mockMergeGithubBranch = vi.fn();
vi.mock("../mergeGithubBranch", () => ({
mergeGithubBranch: (...args: unknown[]) => mockMergeGithubBranch(...args),
}));

const { registerOnMergeTestToMainAction } = await import(
"../handlers/onMergeTestToMainAction"
);

beforeEach(() => {
vi.clearAllMocks();
process.env.GITHUB_TOKEN = "ghp_test";
});

function createMockBot() {
return { onAction: vi.fn() } as any;
}

describe("registerOnMergeTestToMainAction", () => {
it("registers merge_test_to_main: action handler", () => {
const bot = createMockBot();
registerOnMergeTestToMainAction(bot);
expect(bot.onAction).toHaveBeenCalledWith("merge_test_to_main:", expect.any(Function));
});

it("merges test to main and posts success", async () => {
mockMergeGithubBranch.mockResolvedValue({ ok: true });

const bot = createMockBot();
registerOnMergeTestToMainAction(bot);
const handler = bot.onAction.mock.calls[0][1];

const mockThread = { post: vi.fn() };

await handler({ thread: mockThread, actionId: "merge_test_to_main:recoupable/chat" });

expect(mockMergeGithubBranch).toHaveBeenCalledWith("recoupable/chat", "test", "main", "ghp_test");
expect(mockThread.post).toHaveBeenCalledWith("✅ Merged test → main for recoupable/chat.");
});

it("posts error message on failure", async () => {
mockMergeGithubBranch.mockResolvedValue({ ok: false, message: "Merge conflict" });

const bot = createMockBot();
registerOnMergeTestToMainAction(bot);
const handler = bot.onAction.mock.calls[0][1];

const mockThread = { post: vi.fn() };

await handler({ thread: mockThread, actionId: "merge_test_to_main:recoupable/api" });

expect(mockThread.post).toHaveBeenCalledWith(
"❌ Failed to merge test → main for recoupable/api: Merge conflict",
);
});

it("posts missing token message when GITHUB_TOKEN is not set", async () => {
delete process.env.GITHUB_TOKEN;

const bot = createMockBot();
registerOnMergeTestToMainAction(bot);
const handler = bot.onAction.mock.calls[0][1];

const mockThread = { post: vi.fn() };

await handler({ thread: mockThread, actionId: "merge_test_to_main:recoupable/api" });

expect(mockThread.post).toHaveBeenCalledWith("Missing GITHUB_TOKEN — cannot merge branches.");
expect(mockMergeGithubBranch).not.toHaveBeenCalled();
});
});
28 changes: 28 additions & 0 deletions lib/coding-agent/__tests__/parseMergeTestToMainActionId.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { describe, it, expect } from "vitest";
import { parseMergeTestToMainActionId } from "../parseMergeTestToMainActionId";

describe("parseMergeTestToMainActionId", () => {
it("parses a valid action ID", () => {
expect(parseMergeTestToMainActionId("merge_test_to_main:recoupable/api")).toBe("recoupable/api");
});

it("parses action ID with hyphenated repo name", () => {
expect(parseMergeTestToMainActionId("merge_test_to_main:org/sub-repo")).toBe("org/sub-repo");
});

it("returns null for missing repo", () => {
expect(parseMergeTestToMainActionId("merge_test_to_main:")).toBeNull();
});

it("returns null for repo without slash", () => {
expect(parseMergeTestToMainActionId("merge_test_to_main:noslash")).toBeNull();
});

it("returns null for wrong prefix", () => {
expect(parseMergeTestToMainActionId("other_action:repo/name")).toBeNull();
});

it("returns null for empty string", () => {
expect(parseMergeTestToMainActionId("")).toBeNull();
});
});
22 changes: 22 additions & 0 deletions lib/coding-agent/buildMergeTestToMainCard.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { Card, CardText, Actions, Button } from "chat";

/**
* Builds a Card with a "Merge test to main" button for a specific repo.
*
* @param repo - Full repo identifier (e.g. "recoupable/chat")
*/
export function buildMergeTestToMainCard(repo: string) {
return Card({
title: "Merge to Main",
children: [
CardText(`Ready to promote \`test\` → \`main\` for ${repo}.`),
Actions([
Button({
id: `merge_test_to_main:${repo}`,
label: `Merge test → main (${repo})`,
style: "primary",
}),
]),
],
});
}
10 changes: 10 additions & 0 deletions lib/coding-agent/handlers/onMergeAction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,16 @@ import type { CodingAgentThreadState } from "../types";
import { handleMergeSuccess } from "../handleMergeSuccess";
import { parseMergeActionId } from "../parseMergeActionId";
import { mergeGithubPR } from "../mergeGithubPR";
import { buildMergeTestToMainCard } from "../buildMergeTestToMainCard";

/**
* Registers individual per-PR merge button action handlers on the bot.
* Each button has an ID like "merge_pr:<repo>#<number>" and squash-merges
* that single PR via the GitHub API.
*
* When a PR targeting the "test" branch is merged, a follow-up
* "Merge test to main" button is presented.
*
* Uses a prefix pattern so a single handler covers all merge_pr:* actions.
*
* @param bot
Expand Down Expand Up @@ -65,5 +69,11 @@ export function registerOnMergeAction(bot: CodingAgentBot) {
}

await thread.post(`✅ ${pr.repo}#${pr.number} merged.`);

// Offer "Merge test to main" when the PR targeted the test branch
if (pr.baseBranch === "test") {
const card = buildMergeTestToMainCard(pr.repo);
await thread.post({ card });
}
});
}
37 changes: 37 additions & 0 deletions lib/coding-agent/handlers/onMergeTestToMainAction.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import type { CodingAgentBot } from "../bot";
import { mergeGithubBranch } from "../mergeGithubBranch";
import { parseMergeTestToMainActionId } from "../parseMergeTestToMainActionId";

/**
* Registers the "Merge test to main" button action handler on the bot.
* Merges the test branch into main for the specified repo via the GitHub API.
*
* @param bot
*/
export function registerOnMergeTestToMainAction(bot: CodingAgentBot) {
bot.onAction("merge_test_to_main:", async event => {
const thread = event.thread;

const repo = parseMergeTestToMainActionId(event.actionId);
if (!repo) {
await thread.post("Invalid merge test to main action.");
return;
}

const token = process.env.GITHUB_TOKEN;
if (!token) {
await thread.post("Missing GITHUB_TOKEN — cannot merge branches.");
return;
}

const result = await mergeGithubBranch(repo, "test", "main", token);

if (result.ok === false) {
const { message } = result;
await thread.post(`❌ Failed to merge test → main for ${repo}: ${message}`);
return;
}

await thread.post(`✅ Merged test → main for ${repo}.`);
});
}
2 changes: 2 additions & 0 deletions lib/coding-agent/handlers/registerHandlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { codingAgentBot } from "../bot";
import { registerOnNewMention } from "./onNewMention";
import { registerOnSubscribedMessage } from "./onSubscribedMessage";
import { registerOnMergeAction } from "./onMergeAction";
import { registerOnMergeTestToMainAction } from "./onMergeTestToMainAction";

/**
* Registers all coding agent event handlers on the bot singleton.
Expand All @@ -10,3 +11,4 @@ import { registerOnMergeAction } from "./onMergeAction";
registerOnNewMention(codingAgentBot);
registerOnSubscribedMessage(codingAgentBot);
registerOnMergeAction(codingAgentBot);
registerOnMergeTestToMainAction(codingAgentBot);
55 changes: 55 additions & 0 deletions lib/coding-agent/mergeGithubBranch.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
export interface MergeGithubBranchSuccess {
ok: true;
}

export interface MergeGithubBranchFailure {
ok: false;
message: string;
}

export type MergeGithubBranchResult = MergeGithubBranchSuccess | MergeGithubBranchFailure;

/**
* Merges one branch into another via the GitHub API.
*
* @param repo - Full repo identifier (e.g. "recoupable/api")
* @param head - Branch to merge from (e.g. "test")
* @param base - Branch to merge into (e.g. "main")
* @param token - GitHub API token
*/
export async function mergeGithubBranch(
repo: string,
head: string,
base: string,
token: string,
): Promise<MergeGithubBranchResult> {
const [owner, repoName] = repo.split("/");
const response = await fetch(
`https://api.github.com/repos/${owner}/${repoName}/merges`,
{
method: "POST",
headers: {
Authorization: `Bearer ${token}`,
Accept: "application/vnd.github+json",
"X-GitHub-Api-Version": "2022-11-28",
},
body: JSON.stringify({
base,
head,
commit_message: `Merge ${head} into ${base}`,
}),
},
);

// 201 = merged, 204 = already up to date (both are success)
if (response.ok) {
return { ok: true };
}

const errorBody = await response.text();
console.error(
`[coding-agent] branch merge failed for ${repo} (${head} → ${base}): ${response.status} ${errorBody}`,
);
const error = JSON.parse(errorBody);
return { ok: false, message: error.message };
Comment on lines +49 to +54
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing try-catch around JSON.parse can cause unhandled exceptions.

If the GitHub API returns a non-JSON response (e.g., HTML error page during outage, proxy errors, or rate-limit pages), JSON.parse(errorBody) will throw a SyntaxError, crashing the handler and leaving the user without feedback.

🐛 Proposed fix with fallback for non-JSON responses
   const errorBody = await response.text();
   console.error(
     `[coding-agent] branch merge failed for ${repo} (${head} → ${base}): ${response.status} ${errorBody}`,
   );
-  const error = JSON.parse(errorBody);
-  return { ok: false, message: error.message };
+  try {
+    const error = JSON.parse(errorBody);
+    return { ok: false, message: error.message ?? errorBody };
+  } catch {
+    return { ok: false, message: `GitHub API error: ${response.status}` };
+  }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/coding-agent/mergeGithubBranch.ts` around lines 49 - 54, The branch-merge
error handler currently calls JSON.parse(errorBody) without protection; wrap
that parse in a try-catch inside the mergeGithubBranch error handling block so
non-JSON responses (HTML, proxy pages, rate-limit text) don't throw; if
JSON.parse succeeds extract error.message, otherwise fall back to using the raw
errorBody (or a generic message) when returning { ok: false, message: ... } and
ensure the process log still includes the original errorBody; update the code
around the existing variables response, errorBody, and the return that builds
the message accordingly.

}
10 changes: 10 additions & 0 deletions lib/coding-agent/parseMergeTestToMainActionId.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* Parses a merge_test_to_main action ID like "merge_test_to_main:recoupable/api"
* into the repo string, or null if the format doesn't match.
*/
export function parseMergeTestToMainActionId(actionId: string): string | null {
const prefix = "merge_test_to_main:";
if (!actionId.startsWith(prefix)) return null;
const repo = actionId.slice(prefix.length);
return repo.includes("/") ? repo : null;
}