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 <ashwin-ant@users.noreply.github.com>

* prettier

* tsc

* clean up comments

---------

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-authored-by: ashwin-ant <ashwin-ant@users.noreply.github.com>
This commit is contained in:
Ashwin Bhat
2025-06-02 12:15:25 -07:00
committed by GitHub
parent e409c57d90
commit 1d4d6c4b93
10 changed files with 704 additions and 205 deletions

View File

@@ -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");

View File

@@ -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());

View File

@@ -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",
});
});
});