From 1d4d6c4b93f4ca8a7beb52af8bb1034add5353d0 Mon Sep 17 00:00:00 2001 From: Ashwin Bhat Date: Mon, 2 Jun 2025 12:15:25 -0700 Subject: [PATCH] feat: add unified update_claude_comment tool (#98) * feat: add unified update_claude_comment tool - Add new update_claude_comment tool that automatically handles both issue and PR comments - Remove individual update_issue_comment and update_pull_request_comment tools - Pass CLAUDE_COMMENT_ID, GITHUB_EVENT_NAME, and IS_PR to MCP server environment - Simplify Claude's comment update workflow by removing need for owner/repo/commentId params - Update prompts and tests to use the new unified tool * feat: add unified update_claude_comment tool - Add new update_claude_comment tool that automatically handles both issue and PR comments - Remove individual update_issue_comment and update_pull_request_comment tools - Pass CLAUDE_COMMENT_ID, GITHUB_EVENT_NAME, and IS_PR to MCP server environment - Use Octokit instead of raw fetch for better type safety and error handling - Simplify Claude's comment update workflow by removing need for owner/repo/commentId params - Update prompts and tests to use the new unified tool * refactor: extract update_claude_comment logic to standalone testable function - Create new updateClaudeComment function in operations/comments - Add comprehensive unit tests following image-downloader pattern - Update MCP server to use extracted function - Refactor update-comment-link.ts and update-with-branch.ts to eliminate duplication - All tests passing (10 new tests for update-claude-comment) Co-authored-by: ashwin-ant * prettier * tsc * clean up comments --------- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Co-authored-by: ashwin-ant --- src/create-prompt/index.ts | 54 +-- src/entrypoints/prepare.ts | 11 +- src/entrypoints/update-comment-link.ts | 24 +- .../comments/update-claude-comment.ts | 70 +++ .../operations/comments/update-with-branch.ts | 33 +- src/mcp/github-file-ops-server.ts | 65 +++ src/mcp/install-mcp-server.ts | 26 +- test/create-prompt.test.ts | 37 +- test/install-mcp-server.test.ts | 176 ++++---- test/update-claude-comment.test.ts | 413 ++++++++++++++++++ 10 files changed, 704 insertions(+), 205 deletions(-) create mode 100644 src/github/operations/comments/update-claude-comment.ts create mode 100644 test/update-claude-comment.test.ts diff --git a/src/create-prompt/index.ts b/src/create-prompt/index.ts index 93bca54..e292f34 100644 --- a/src/create-prompt/index.ts +++ b/src/create-prompt/index.ts @@ -31,24 +31,13 @@ const BASE_ALLOWED_TOOLS = [ "Write", "mcp__github_file_ops__commit_files", "mcp__github_file_ops__delete_files", + "mcp__github_file_ops__update_claude_comment", ]; const DISALLOWED_TOOLS = ["WebSearch", "WebFetch"]; -export function buildAllowedToolsString( - eventData: EventData, - customAllowedTools?: string, -): string { +export function buildAllowedToolsString(customAllowedTools?: string): string { let baseTools = [...BASE_ALLOWED_TOOLS]; - // Add the appropriate comment tool based on event type - if (eventData.eventName === "pull_request_review_comment") { - // For inline PR review comments, only use PR comment tool - baseTools.push("mcp__github__update_pull_request_comment"); - } else { - // For all other events (issue comments, PR reviews, issues), use issue comment tool - baseTools.push("mcp__github__update_issue_comment"); - } - let allAllowedTools = baseTools.join(","); if (customAllowedTools) { allAllowedTools = `${allAllowedTools},${customAllowedTools}`; @@ -447,33 +436,15 @@ ${sanitizeContent(context.directPrompt)} ` : "" } -${ - eventData.eventName === "pull_request_review_comment" - ? ` -IMPORTANT: For this inline PR review comment, you have been provided with ONLY the mcp__github__update_pull_request_comment tool to update this specific review comment. +${` +IMPORTANT: You have been provided with the mcp__github_file_ops__update_claude_comment tool to update your comment. This tool automatically handles both issue and PR comments. -Tool usage example for mcp__github__update_pull_request_comment: +Tool usage example for mcp__github_file_ops__update_claude_comment: { - "owner": "${context.repository.split("/")[0]}", - "repo": "${context.repository.split("/")[1]}", - "commentId": ${eventData.commentId || context.claudeCommentId}, "body": "Your comment text here" } -All four parameters (owner, repo, commentId, body) are required. -` - : ` -IMPORTANT: For this event type, you have been provided with ONLY the mcp__github__update_issue_comment tool to update comments. - -Tool usage example for mcp__github__update_issue_comment: -{ - "owner": "${context.repository.split("/")[0]}", - "repo": "${context.repository.split("/")[1]}", - "commentId": ${context.claudeCommentId}, - "body": "Your comment text here" -} -All four parameters (owner, repo, commentId, body) are required. -` -} +Only the body parameter is required - the tool automatically knows which comment to update. +`} Your task is to analyze the context, understand the request, and provide helpful responses and/or implement code changes as needed. @@ -487,7 +458,7 @@ Follow these steps: 1. Create a Todo List: - Use your GitHub comment to maintain a detailed task list based on the request. - Format todos as a checklist (- [ ] for incomplete, - [x] for complete). - - Update the comment using ${eventData.eventName === "pull_request_review_comment" ? "mcp__github__update_pull_request_comment" : "mcp__github__update_issue_comment"} with each task completion. + - Update the comment using mcp__github_file_ops__update_claude_comment with each task completion. 2. Gather Context: - Analyze the pre-fetched data provided above. @@ -517,11 +488,11 @@ ${context.directPrompt ? ` - DIRECT INSTRUCTION: A direct instruction was prov - Look for bugs, security issues, performance problems, and other issues - Suggest improvements for readability and maintainability - Check for best practices and coding standards - - Reference specific code sections with file paths and line numbers${eventData.isPR ? "\n - AFTER reading files and analyzing code, you MUST call mcp__github__update_issue_comment to post your review" : ""} + - Reference specific code sections with file paths and line numbers${eventData.isPR ? "\n - AFTER reading files and analyzing code, you MUST call mcp__github_file_ops__update_claude_comment to post your review" : ""} - Formulate a concise, technical, and helpful response based on the context. - Reference specific code with inline formatting or code blocks. - Include relevant file paths and line numbers when applicable. - - ${eventData.isPR ? "IMPORTANT: Submit your review feedback by updating the Claude comment. This will be displayed as your PR review." : "Remember that this feedback must be posted to the GitHub comment."} + - ${eventData.isPR ? "IMPORTANT: Submit your review feedback by updating the Claude comment using mcp__github_file_ops__update_claude_comment. This will be displayed as your PR review." : "Remember that this feedback must be posted to the GitHub comment using mcp__github_file_ops__update_claude_comment."} B. For Straightforward Changes: - Use file system tools to make the change locally. @@ -576,8 +547,8 @@ ${context.directPrompt ? ` - DIRECT INSTRUCTION: A direct instruction was prov Important Notes: - All communication must happen through GitHub PR comments. -- Never create new comments. Only update the existing comment using ${eventData.eventName === "pull_request_review_comment" ? "mcp__github__update_pull_request_comment" : "mcp__github__update_issue_comment"} with comment_id: ${context.claudeCommentId}. -- This includes ALL responses: code reviews, answers to questions, progress updates, and final results.${eventData.isPR ? "\n- PR CRITICAL: After reading files and forming your response, you MUST post it by calling mcp__github__update_issue_comment. Do NOT just respond with a normal response, the user will not see it." : ""} +- Never create new comments. Only update the existing comment using mcp__github_file_ops__update_claude_comment. +- This includes ALL responses: code reviews, answers to questions, progress updates, and final results.${eventData.isPR ? "\n- PR CRITICAL: After reading files and forming your response, you MUST post it by calling mcp__github_file_ops__update_claude_comment. Do NOT just respond with a normal response, the user will not see it." : ""} - You communicate exclusively by editing your single comment - not through any other means. - Use this spinner HTML when work is in progress: ${eventData.isPR && !eventData.claudeBranch ? `- Always push to the existing branch when triggered on a PR.` : `- IMPORTANT: You are already on the correct branch (${eventData.claudeBranch || "the created branch"}). Never create new branches when triggered on issues or closed/merged PRs.`} @@ -665,7 +636,6 @@ export async function createPrompt( // Set allowed tools const allAllowedTools = buildAllowedToolsString( - preparedContext.eventData, preparedContext.allowedTools, ); const allDisallowedTools = buildDisallowedToolsString( diff --git a/src/entrypoints/prepare.ts b/src/entrypoints/prepare.ts index 006a62e..5736268 100644 --- a/src/entrypoints/prepare.ts +++ b/src/entrypoints/prepare.ts @@ -85,13 +85,14 @@ async function run() { // Step 11: Get MCP configuration const additionalMcpConfig = process.env.MCP_CONFIG || ""; - const mcpConfig = await prepareMcpConfig( + const mcpConfig = await prepareMcpConfig({ githubToken, - context.repository.owner, - context.repository.repo, - branchInfo.currentBranch, + owner: context.repository.owner, + repo: context.repository.repo, + branch: branchInfo.currentBranch, additionalMcpConfig, - ); + claudeCommentId: commentId.toString(), + }); core.setOutput("mcp_config", mcpConfig); } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); diff --git a/src/entrypoints/update-comment-link.ts b/src/entrypoints/update-comment-link.ts index 2dfd8c9..9090373 100644 --- a/src/entrypoints/update-comment-link.ts +++ b/src/entrypoints/update-comment-link.ts @@ -12,6 +12,7 @@ import { } from "../github/context"; import { GITHUB_SERVER_URL } from "../github/api/config"; import { checkAndDeleteEmptyBranch } from "../github/operations/branch-cleanup"; +import { updateClaudeComment } from "../github/operations/comments/update-claude-comment"; async function run() { try { @@ -204,23 +205,14 @@ async function run() { const updatedBody = updateCommentBody(commentInput); - // Update the comment using the appropriate API try { - if (isPRReviewComment) { - await octokit.rest.pulls.updateReviewComment({ - owner, - repo, - comment_id: commentId, - body: updatedBody, - }); - } else { - await octokit.rest.issues.updateComment({ - owner, - repo, - comment_id: commentId, - body: updatedBody, - }); - } + await updateClaudeComment(octokit.rest, { + owner, + repo, + commentId, + body: updatedBody, + isPullRequestReviewComment: isPRReviewComment, + }); console.log( `✅ Updated ${isPRReviewComment ? "PR review" : "issue"} comment ${commentId} with job link`, ); diff --git a/src/github/operations/comments/update-claude-comment.ts b/src/github/operations/comments/update-claude-comment.ts new file mode 100644 index 0000000..d6f4bd1 --- /dev/null +++ b/src/github/operations/comments/update-claude-comment.ts @@ -0,0 +1,70 @@ +import { Octokit } from "@octokit/rest"; + +export type UpdateClaudeCommentParams = { + owner: string; + repo: string; + commentId: number; + body: string; + isPullRequestReviewComment: boolean; +}; + +export type UpdateClaudeCommentResult = { + id: number; + html_url: string; + updated_at: string; +}; + +/** + * Updates a Claude comment on GitHub (either an issue/PR comment or a PR review comment) + * + * @param octokit - Authenticated Octokit instance + * @param params - Parameters for updating the comment + * @returns The updated comment details + * @throws Error if the update fails + */ +export async function updateClaudeComment( + octokit: Octokit, + params: UpdateClaudeCommentParams, +): Promise { + const { owner, repo, commentId, body, isPullRequestReviewComment } = params; + + let response; + + try { + if (isPullRequestReviewComment) { + // Try PR review comment API first + response = await octokit.rest.pulls.updateReviewComment({ + owner, + repo, + comment_id: commentId, + body, + }); + } else { + // Use issue comment API (works for both issues and PR general comments) + response = await octokit.rest.issues.updateComment({ + owner, + repo, + comment_id: commentId, + body, + }); + } + } catch (error: any) { + // If PR review comment update fails with 404, fall back to issue comment API + if (isPullRequestReviewComment && error.status === 404) { + response = await octokit.rest.issues.updateComment({ + owner, + repo, + comment_id: commentId, + body, + }); + } else { + throw error; + } + } + + return { + id: response.data.id, + html_url: response.data.html_url, + updated_at: response.data.updated_at, + }; +} diff --git a/src/github/operations/comments/update-with-branch.ts b/src/github/operations/comments/update-with-branch.ts index 6709bac..838b154 100644 --- a/src/github/operations/comments/update-with-branch.ts +++ b/src/github/operations/comments/update-with-branch.ts @@ -15,6 +15,7 @@ import { isPullRequestReviewCommentEvent, type ParsedGitHubContext, } from "../../context"; +import { updateClaudeComment } from "./update-claude-comment"; export async function updateTrackingComment( octokit: Octokits, @@ -36,25 +37,19 @@ export async function updateTrackingComment( // Update the existing comment with the branch link try { - if (isPullRequestReviewCommentEvent(context)) { - // For PR review comments (inline comments), use the pulls API - await octokit.rest.pulls.updateReviewComment({ - owner, - repo, - comment_id: commentId, - body: updatedBody, - }); - console.log(`✅ Updated PR review comment ${commentId} with branch link`); - } else { - // For all other comments, use the issues API - await octokit.rest.issues.updateComment({ - owner, - repo, - comment_id: commentId, - body: updatedBody, - }); - console.log(`✅ Updated issue comment ${commentId} with branch link`); - } + const isPRReviewComment = isPullRequestReviewCommentEvent(context); + + await updateClaudeComment(octokit.rest, { + owner, + repo, + commentId, + body: updatedBody, + isPullRequestReviewComment: isPRReviewComment, + }); + + console.log( + `✅ Updated ${isPRReviewComment ? "PR review" : "issue"} comment ${commentId} with branch link`, + ); } catch (error) { console.error("Error updating comment with branch link:", error); throw error; diff --git a/src/mcp/github-file-ops-server.ts b/src/mcp/github-file-ops-server.ts index 19834c9..a34f115 100644 --- a/src/mcp/github-file-ops-server.ts +++ b/src/mcp/github-file-ops-server.ts @@ -7,6 +7,8 @@ import { readFile } from "fs/promises"; import { join } from "path"; import fetch from "node-fetch"; import { GITHUB_API_URL } from "../github/api/config"; +import { Octokit } from "@octokit/rest"; +import { updateClaudeComment } from "../github/operations/comments/update-claude-comment"; type GitHubRef = { object: { @@ -439,6 +441,69 @@ server.tool( }, ); +server.tool( + "update_claude_comment", + "Update the Claude comment with progress and results (automatically handles both issue and PR comments)", + { + body: z.string().describe("The updated comment content"), + }, + async ({ body }) => { + try { + const githubToken = process.env.GITHUB_TOKEN; + const claudeCommentId = process.env.CLAUDE_COMMENT_ID; + const eventName = process.env.GITHUB_EVENT_NAME; + + if (!githubToken) { + throw new Error("GITHUB_TOKEN environment variable is required"); + } + if (!claudeCommentId) { + throw new Error("CLAUDE_COMMENT_ID environment variable is required"); + } + + const owner = REPO_OWNER; + const repo = REPO_NAME; + const commentId = parseInt(claudeCommentId, 10); + + const octokit = new Octokit({ + auth: githubToken, + }); + + const isPullRequestReviewComment = + eventName === "pull_request_review_comment"; + + const result = await updateClaudeComment(octokit, { + owner, + repo, + commentId, + body, + isPullRequestReviewComment, + }); + + return { + content: [ + { + type: "text", + text: JSON.stringify(result, null, 2), + }, + ], + }; + } catch (error) { + const errorMessage = + error instanceof Error ? error.message : String(error); + return { + content: [ + { + type: "text", + text: `Error: ${errorMessage}`, + }, + ], + error: errorMessage, + isError: true, + }; + } + }, +); + async function runServer() { const transport = new StdioServerTransport(); await server.connect(transport); diff --git a/src/mcp/install-mcp-server.ts b/src/mcp/install-mcp-server.ts index 4a4921b..251d2a0 100644 --- a/src/mcp/install-mcp-server.ts +++ b/src/mcp/install-mcp-server.ts @@ -1,12 +1,25 @@ import * as core from "@actions/core"; +type PrepareConfigParams = { + githubToken: string; + owner: string; + repo: string; + branch: string; + additionalMcpConfig?: string; + claudeCommentId?: string; +}; + export async function prepareMcpConfig( - githubToken: string, - owner: string, - repo: string, - branch: string, - additionalMcpConfig?: string, + params: PrepareConfigParams, ): Promise { + const { + githubToken, + owner, + repo, + branch, + additionalMcpConfig, + claudeCommentId, + } = params; try { const baseMcpConfig = { mcpServers: { @@ -36,6 +49,9 @@ export async function prepareMcpConfig( REPO_NAME: repo, BRANCH_NAME: branch, REPO_DIR: process.env.GITHUB_WORKSPACE || process.cwd(), + ...(claudeCommentId && { CLAUDE_COMMENT_ID: claudeCommentId }), + GITHUB_EVENT_NAME: process.env.GITHUB_EVENT_NAME || "", + IS_PR: process.env.IS_PR || "false", }, }, }, diff --git a/test/create-prompt.test.ts b/test/create-prompt.test.ts index 94064b6..617f0ac 100644 --- a/test/create-prompt.test.ts +++ b/test/create-prompt.test.ts @@ -8,7 +8,6 @@ import { buildDisallowedToolsString, } from "../src/create-prompt"; import type { PreparedContext } from "../src/create-prompt"; -import type { EventData } from "../src/create-prompt/types"; describe("generatePrompt", () => { const mockGitHubData = { @@ -619,15 +618,7 @@ describe("getEventTypeAndContext", () => { describe("buildAllowedToolsString", () => { test("should return issue comment tool for regular events", () => { - const mockEventData: EventData = { - eventName: "issue_comment", - commentId: "123", - isPR: true, - prNumber: "456", - commentBody: "Test comment", - }; - - const result = buildAllowedToolsString(mockEventData); + const result = buildAllowedToolsString(); // The base tools should be in the result expect(result).toContain("Edit"); @@ -636,22 +627,15 @@ describe("buildAllowedToolsString", () => { expect(result).toContain("LS"); expect(result).toContain("Read"); expect(result).toContain("Write"); - expect(result).toContain("mcp__github__update_issue_comment"); + expect(result).toContain("mcp__github_file_ops__update_claude_comment"); + expect(result).not.toContain("mcp__github__update_issue_comment"); expect(result).not.toContain("mcp__github__update_pull_request_comment"); expect(result).toContain("mcp__github_file_ops__commit_files"); expect(result).toContain("mcp__github_file_ops__delete_files"); }); test("should return PR comment tool for inline review comments", () => { - const mockEventData: EventData = { - eventName: "pull_request_review_comment", - isPR: true, - prNumber: "456", - commentBody: "Test review comment", - commentId: "789", - }; - - const result = buildAllowedToolsString(mockEventData); + const result = buildAllowedToolsString(); // The base tools should be in the result expect(result).toContain("Edit"); @@ -660,23 +644,16 @@ describe("buildAllowedToolsString", () => { expect(result).toContain("LS"); expect(result).toContain("Read"); expect(result).toContain("Write"); + expect(result).toContain("mcp__github_file_ops__update_claude_comment"); expect(result).not.toContain("mcp__github__update_issue_comment"); - expect(result).toContain("mcp__github__update_pull_request_comment"); + expect(result).not.toContain("mcp__github__update_pull_request_comment"); expect(result).toContain("mcp__github_file_ops__commit_files"); expect(result).toContain("mcp__github_file_ops__delete_files"); }); test("should append custom tools when provided", () => { - const mockEventData: EventData = { - eventName: "issue_comment", - commentId: "123", - isPR: true, - prNumber: "456", - commentBody: "Test comment", - }; - const customTools = "Tool1,Tool2,Tool3"; - const result = buildAllowedToolsString(mockEventData, customTools); + const result = buildAllowedToolsString(customTools); // Base tools should be present expect(result).toContain("Edit"); diff --git a/test/install-mcp-server.test.ts b/test/install-mcp-server.test.ts index 5a93aa6..3d2f02e 100644 --- a/test/install-mcp-server.test.ts +++ b/test/install-mcp-server.test.ts @@ -25,12 +25,12 @@ describe("prepareMcpConfig", () => { }); test("should return base config when no additional config is provided", async () => { - const result = await prepareMcpConfig( - "test-token", - "test-owner", - "test-repo", - "test-branch", - ); + const result = await prepareMcpConfig({ + githubToken: "test-token", + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + }); const parsed = JSON.parse(result); expect(parsed.mcpServers).toBeDefined(); @@ -50,13 +50,13 @@ describe("prepareMcpConfig", () => { }); test("should return base config when additional config is empty string", async () => { - const result = await prepareMcpConfig( - "test-token", - "test-owner", - "test-repo", - "test-branch", - "", - ); + const result = await prepareMcpConfig({ + githubToken: "test-token", + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + additionalMcpConfig: "", + }); const parsed = JSON.parse(result); expect(parsed.mcpServers).toBeDefined(); @@ -66,13 +66,13 @@ describe("prepareMcpConfig", () => { }); test("should return base config when additional config is whitespace only", async () => { - const result = await prepareMcpConfig( - "test-token", - "test-owner", - "test-repo", - "test-branch", - " \n\t ", - ); + const result = await prepareMcpConfig({ + githubToken: "test-token", + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + additionalMcpConfig: " \n\t ", + }); const parsed = JSON.parse(result); expect(parsed.mcpServers).toBeDefined(); @@ -94,13 +94,13 @@ describe("prepareMcpConfig", () => { }, }); - const result = await prepareMcpConfig( - "test-token", - "test-owner", - "test-repo", - "test-branch", - additionalConfig, - ); + const result = await prepareMcpConfig({ + githubToken: "test-token", + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + additionalMcpConfig: additionalConfig, + }); const parsed = JSON.parse(result); expect(consoleInfoSpy).toHaveBeenCalledWith( @@ -127,13 +127,13 @@ describe("prepareMcpConfig", () => { }, }); - const result = await prepareMcpConfig( - "test-token", - "test-owner", - "test-repo", - "test-branch", - additionalConfig, - ); + const result = await prepareMcpConfig({ + githubToken: "test-token", + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + additionalMcpConfig: additionalConfig, + }); const parsed = JSON.parse(result); expect(consoleInfoSpy).toHaveBeenCalledWith( @@ -163,13 +163,13 @@ describe("prepareMcpConfig", () => { }, }); - const result = await prepareMcpConfig( - "test-token", - "test-owner", - "test-repo", - "test-branch", - additionalConfig, - ); + const result = await prepareMcpConfig({ + githubToken: "test-token", + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + additionalMcpConfig: additionalConfig, + }); const parsed = JSON.parse(result); expect(parsed.customProperty).toBe("custom-value"); @@ -181,13 +181,13 @@ describe("prepareMcpConfig", () => { test("should handle invalid JSON gracefully", async () => { const invalidJson = "{ invalid json }"; - const result = await prepareMcpConfig( - "test-token", - "test-owner", - "test-repo", - "test-branch", - invalidJson, - ); + const result = await prepareMcpConfig({ + githubToken: "test-token", + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + additionalMcpConfig: invalidJson, + }); const parsed = JSON.parse(result); expect(consoleWarningSpy).toHaveBeenCalledWith( @@ -200,13 +200,13 @@ describe("prepareMcpConfig", () => { test("should handle non-object JSON values", async () => { const nonObjectJson = JSON.stringify("string value"); - const result = await prepareMcpConfig( - "test-token", - "test-owner", - "test-repo", - "test-branch", - nonObjectJson, - ); + const result = await prepareMcpConfig({ + githubToken: "test-token", + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + additionalMcpConfig: nonObjectJson, + }); const parsed = JSON.parse(result); expect(consoleWarningSpy).toHaveBeenCalledWith( @@ -222,13 +222,13 @@ describe("prepareMcpConfig", () => { test("should handle null JSON value", async () => { const nullJson = JSON.stringify(null); - const result = await prepareMcpConfig( - "test-token", - "test-owner", - "test-repo", - "test-branch", - nullJson, - ); + const result = await prepareMcpConfig({ + githubToken: "test-token", + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + additionalMcpConfig: nullJson, + }); const parsed = JSON.parse(result); expect(consoleWarningSpy).toHaveBeenCalledWith( @@ -244,13 +244,13 @@ describe("prepareMcpConfig", () => { test("should handle array JSON value", async () => { const arrayJson = JSON.stringify([1, 2, 3]); - const result = await prepareMcpConfig( - "test-token", - "test-owner", - "test-repo", - "test-branch", - arrayJson, - ); + const result = await prepareMcpConfig({ + githubToken: "test-token", + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + additionalMcpConfig: arrayJson, + }); const parsed = JSON.parse(result); // Arrays are objects in JavaScript, so they pass the object check @@ -289,13 +289,13 @@ describe("prepareMcpConfig", () => { }, }); - const result = await prepareMcpConfig( - "test-token", - "test-owner", - "test-repo", - "test-branch", - additionalConfig, - ); + const result = await prepareMcpConfig({ + githubToken: "test-token", + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + additionalMcpConfig: additionalConfig, + }); const parsed = JSON.parse(result); expect(parsed.mcpServers.server1).toBeDefined(); @@ -310,12 +310,12 @@ describe("prepareMcpConfig", () => { const oldEnv = process.env.GITHUB_ACTION_PATH; process.env.GITHUB_ACTION_PATH = "/test/action/path"; - const result = await prepareMcpConfig( - "test-token", - "test-owner", - "test-repo", - "test-branch", - ); + const result = await prepareMcpConfig({ + githubToken: "test-token", + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + }); const parsed = JSON.parse(result); expect(parsed.mcpServers.github_file_ops.args[1]).toBe( @@ -329,12 +329,12 @@ describe("prepareMcpConfig", () => { const oldEnv = process.env.GITHUB_WORKSPACE; delete process.env.GITHUB_WORKSPACE; - const result = await prepareMcpConfig( - "test-token", - "test-owner", - "test-repo", - "test-branch", - ); + const result = await prepareMcpConfig({ + githubToken: "test-token", + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + }); const parsed = JSON.parse(result); expect(parsed.mcpServers.github_file_ops.env.REPO_DIR).toBe(process.cwd()); diff --git a/test/update-claude-comment.test.ts b/test/update-claude-comment.test.ts new file mode 100644 index 0000000..e56c039 --- /dev/null +++ b/test/update-claude-comment.test.ts @@ -0,0 +1,413 @@ +import { describe, test, expect, jest, beforeEach } from "bun:test"; +import { Octokit } from "@octokit/rest"; +import { + updateClaudeComment, + type UpdateClaudeCommentParams, +} from "../src/github/operations/comments/update-claude-comment"; + +describe("updateClaudeComment", () => { + let mockOctokit: Octokit; + + beforeEach(() => { + mockOctokit = { + rest: { + issues: { + updateComment: jest.fn(), + }, + pulls: { + updateReviewComment: jest.fn(), + }, + }, + } as any as Octokit; + }); + + test("should update issue comment successfully", async () => { + const mockResponse = { + data: { + id: 123456, + html_url: "https://github.com/owner/repo/issues/1#issuecomment-123456", + updated_at: "2024-01-01T00:00:00Z", + body: "Updated comment", + }, + }; + + // @ts-expect-error Mock implementation doesn't match full type signature + mockOctokit.rest.issues.updateComment = jest + .fn() + .mockResolvedValue(mockResponse); + + const params: UpdateClaudeCommentParams = { + owner: "testowner", + repo: "testrepo", + commentId: 123456, + body: "Updated comment", + isPullRequestReviewComment: false, + }; + + const result = await updateClaudeComment(mockOctokit, params); + + expect(mockOctokit.rest.issues.updateComment).toHaveBeenCalledWith({ + owner: "testowner", + repo: "testrepo", + comment_id: 123456, + body: "Updated comment", + }); + + expect(result).toEqual({ + id: 123456, + html_url: "https://github.com/owner/repo/issues/1#issuecomment-123456", + updated_at: "2024-01-01T00:00:00Z", + }); + }); + + test("should update PR comment successfully", async () => { + const mockResponse = { + data: { + id: 789012, + html_url: "https://github.com/owner/repo/pull/2#issuecomment-789012", + updated_at: "2024-01-02T00:00:00Z", + body: "Updated PR comment", + }, + }; + + // @ts-expect-error Mock implementation doesn't match full type signature + mockOctokit.rest.issues.updateComment = jest + .fn() + .mockResolvedValue(mockResponse); + + const params: UpdateClaudeCommentParams = { + owner: "testowner", + repo: "testrepo", + commentId: 789012, + body: "Updated PR comment", + isPullRequestReviewComment: false, + }; + + const result = await updateClaudeComment(mockOctokit, params); + + expect(mockOctokit.rest.issues.updateComment).toHaveBeenCalledWith({ + owner: "testowner", + repo: "testrepo", + comment_id: 789012, + body: "Updated PR comment", + }); + + expect(result).toEqual({ + id: 789012, + html_url: "https://github.com/owner/repo/pull/2#issuecomment-789012", + updated_at: "2024-01-02T00:00:00Z", + }); + }); + + test("should update PR review comment successfully", async () => { + const mockResponse = { + data: { + id: 345678, + html_url: "https://github.com/owner/repo/pull/3#discussion_r345678", + updated_at: "2024-01-03T00:00:00Z", + body: "Updated review comment", + }, + }; + + // @ts-expect-error Mock implementation doesn't match full type signature + mockOctokit.rest.pulls.updateReviewComment = jest + .fn() + .mockResolvedValue(mockResponse); + + const params: UpdateClaudeCommentParams = { + owner: "testowner", + repo: "testrepo", + commentId: 345678, + body: "Updated review comment", + isPullRequestReviewComment: true, + }; + + const result = await updateClaudeComment(mockOctokit, params); + + expect(mockOctokit.rest.pulls.updateReviewComment).toHaveBeenCalledWith({ + owner: "testowner", + repo: "testrepo", + comment_id: 345678, + body: "Updated review comment", + }); + + expect(result).toEqual({ + id: 345678, + html_url: "https://github.com/owner/repo/pull/3#discussion_r345678", + updated_at: "2024-01-03T00:00:00Z", + }); + }); + + test("should fallback to issue comment API when PR review comment update fails with 404", async () => { + const mockError = new Error("Not Found") as any; + mockError.status = 404; + + const mockResponse = { + data: { + id: 456789, + html_url: "https://github.com/owner/repo/pull/4#issuecomment-456789", + updated_at: "2024-01-04T00:00:00Z", + body: "Updated via fallback", + }, + }; + + // @ts-expect-error Mock implementation doesn't match full type signature + mockOctokit.rest.pulls.updateReviewComment = jest + .fn() + .mockRejectedValue(mockError); + // @ts-expect-error Mock implementation doesn't match full type signature + mockOctokit.rest.issues.updateComment = jest + .fn() + .mockResolvedValue(mockResponse); + + const params: UpdateClaudeCommentParams = { + owner: "testowner", + repo: "testrepo", + commentId: 456789, + body: "Updated via fallback", + isPullRequestReviewComment: true, + }; + + const result = await updateClaudeComment(mockOctokit, params); + + expect(mockOctokit.rest.pulls.updateReviewComment).toHaveBeenCalledWith({ + owner: "testowner", + repo: "testrepo", + comment_id: 456789, + body: "Updated via fallback", + }); + + expect(mockOctokit.rest.issues.updateComment).toHaveBeenCalledWith({ + owner: "testowner", + repo: "testrepo", + comment_id: 456789, + body: "Updated via fallback", + }); + + expect(result).toEqual({ + id: 456789, + html_url: "https://github.com/owner/repo/pull/4#issuecomment-456789", + updated_at: "2024-01-04T00:00:00Z", + }); + }); + + test("should propagate error when PR review comment update fails with non-404 error", async () => { + const mockError = new Error("Internal Server Error") as any; + mockError.status = 500; + + // @ts-expect-error Mock implementation doesn't match full type signature + mockOctokit.rest.pulls.updateReviewComment = jest + .fn() + .mockRejectedValue(mockError); + + const params: UpdateClaudeCommentParams = { + owner: "testowner", + repo: "testrepo", + commentId: 567890, + body: "This will fail", + isPullRequestReviewComment: true, + }; + + await expect(updateClaudeComment(mockOctokit, params)).rejects.toEqual( + mockError, + ); + + expect(mockOctokit.rest.pulls.updateReviewComment).toHaveBeenCalledWith({ + owner: "testowner", + repo: "testrepo", + comment_id: 567890, + body: "This will fail", + }); + + // Ensure fallback wasn't attempted + expect(mockOctokit.rest.issues.updateComment).not.toHaveBeenCalled(); + }); + + test("should propagate error when issue comment update fails", async () => { + const mockError = new Error("Forbidden"); + + // @ts-expect-error Mock implementation doesn't match full type signature + mockOctokit.rest.issues.updateComment = jest + .fn() + .mockRejectedValue(mockError); + + const params: UpdateClaudeCommentParams = { + owner: "testowner", + repo: "testrepo", + commentId: 678901, + body: "This will also fail", + isPullRequestReviewComment: false, + }; + + await expect(updateClaudeComment(mockOctokit, params)).rejects.toEqual( + mockError, + ); + + expect(mockOctokit.rest.issues.updateComment).toHaveBeenCalledWith({ + owner: "testowner", + repo: "testrepo", + comment_id: 678901, + body: "This will also fail", + }); + }); + + test("should handle empty body", async () => { + const mockResponse = { + data: { + id: 111222, + html_url: "https://github.com/owner/repo/issues/5#issuecomment-111222", + updated_at: "2024-01-05T00:00:00Z", + body: "", + }, + }; + + // @ts-expect-error Mock implementation doesn't match full type signature + mockOctokit.rest.issues.updateComment = jest + .fn() + .mockResolvedValue(mockResponse); + + const params: UpdateClaudeCommentParams = { + owner: "testowner", + repo: "testrepo", + commentId: 111222, + body: "", + isPullRequestReviewComment: false, + }; + + const result = await updateClaudeComment(mockOctokit, params); + + expect(result).toEqual({ + id: 111222, + html_url: "https://github.com/owner/repo/issues/5#issuecomment-111222", + updated_at: "2024-01-05T00:00:00Z", + }); + }); + + test("should handle very long body", async () => { + const longBody = "x".repeat(10000); + const mockResponse = { + data: { + id: 333444, + html_url: "https://github.com/owner/repo/issues/6#issuecomment-333444", + updated_at: "2024-01-06T00:00:00Z", + body: longBody, + }, + }; + + // @ts-expect-error Mock implementation doesn't match full type signature + mockOctokit.rest.issues.updateComment = jest + .fn() + .mockResolvedValue(mockResponse); + + const params: UpdateClaudeCommentParams = { + owner: "testowner", + repo: "testrepo", + commentId: 333444, + body: longBody, + isPullRequestReviewComment: false, + }; + + const result = await updateClaudeComment(mockOctokit, params); + + expect(mockOctokit.rest.issues.updateComment).toHaveBeenCalledWith({ + owner: "testowner", + repo: "testrepo", + comment_id: 333444, + body: longBody, + }); + + expect(result).toEqual({ + id: 333444, + html_url: "https://github.com/owner/repo/issues/6#issuecomment-333444", + updated_at: "2024-01-06T00:00:00Z", + }); + }); + + test("should handle markdown formatting in body", async () => { + const markdownBody = ` +# Header +- List item 1 +- List item 2 + +\`\`\`typescript +const code = "example"; +\`\`\` + +[Link](https://example.com) + `.trim(); + + const mockResponse = { + data: { + id: 555666, + html_url: "https://github.com/owner/repo/issues/7#issuecomment-555666", + updated_at: "2024-01-07T00:00:00Z", + body: markdownBody, + }, + }; + + // @ts-expect-error Mock implementation doesn't match full type signature + mockOctokit.rest.issues.updateComment = jest + .fn() + .mockResolvedValue(mockResponse); + + const params: UpdateClaudeCommentParams = { + owner: "testowner", + repo: "testrepo", + commentId: 555666, + body: markdownBody, + isPullRequestReviewComment: false, + }; + + const result = await updateClaudeComment(mockOctokit, params); + + expect(mockOctokit.rest.issues.updateComment).toHaveBeenCalledWith({ + owner: "testowner", + repo: "testrepo", + comment_id: 555666, + body: markdownBody, + }); + + expect(result).toEqual({ + id: 555666, + html_url: "https://github.com/owner/repo/issues/7#issuecomment-555666", + updated_at: "2024-01-07T00:00:00Z", + }); + }); + + test("should handle different response data fields", async () => { + const mockResponse = { + data: { + id: 777888, + html_url: "https://github.com/owner/repo/pull/8#discussion_r777888", + updated_at: "2024-01-08T12:30:45Z", + body: "Updated", + // Additional fields that might be in the response + created_at: "2024-01-01T00:00:00Z", + user: { login: "bot" }, + node_id: "MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDc3Nzg4OA==", + }, + }; + + // @ts-expect-error Mock implementation doesn't match full type signature + mockOctokit.rest.pulls.updateReviewComment = jest + .fn() + .mockResolvedValue(mockResponse); + + const params: UpdateClaudeCommentParams = { + owner: "testowner", + repo: "testrepo", + commentId: 777888, + body: "Updated", + isPullRequestReviewComment: true, + }; + + const result = await updateClaudeComment(mockOctokit, params); + + // Should only return the specific fields we care about + expect(result).toEqual({ + id: 777888, + html_url: "https://github.com/owner/repo/pull/8#discussion_r777888", + updated_at: "2024-01-08T12:30:45Z", + }); + }); +});