diff --git a/base-action/test/inline-comment-utils.test.ts b/base-action/test/inline-comment-utils.test.ts new file mode 100644 index 000000000..43f63dfcb --- /dev/null +++ b/base-action/test/inline-comment-utils.test.ts @@ -0,0 +1,247 @@ +#!/usr/bin/env bun + +import { describe, test, expect } from "bun:test"; +import { + findExistingRootComment, + resolveInReplyTo, + type ReviewComment, +} from "../../src/mcp/inline-comment-utils"; + +describe("inline-comment-utils", () => { + describe("findExistingRootComment", () => { + const mockComments: ReviewComment[] = [ + { + id: 100, + path: "src/index.ts", + line: 42, + original_line: 42, + in_reply_to_id: null, + }, + { + id: 101, + path: "src/index.ts", + line: 42, + original_line: 42, + in_reply_to_id: 100, // This is a reply, not a root comment + }, + { + id: 102, + path: "src/utils.ts", + line: 10, + original_line: 10, + in_reply_to_id: null, + }, + { + id: 103, + path: "src/index.ts", + line: 50, + original_line: 55, // original_line differs from current line + in_reply_to_id: null, + }, + ]; + + test("should find existing root comment on same path and line", () => { + const result = findExistingRootComment(mockComments, "src/index.ts", 42); + expect(result).toBeDefined(); + expect(result?.id).toBe(100); + }); + + test("should not find reply comments (only root comments)", () => { + // Comment 101 is on the same line but it's a reply + const replyComment = mockComments[1]!; + const comments: ReviewComment[] = [replyComment]; + const result = findExistingRootComment(comments, "src/index.ts", 42); + expect(result).toBeUndefined(); + }); + + test("should find comment by original_line when line doesn't match", () => { + const result = findExistingRootComment(mockComments, "src/index.ts", 55); + expect(result).toBeDefined(); + expect(result?.id).toBe(103); + }); + + test("should return undefined when no matching comment exists", () => { + const result = findExistingRootComment(mockComments, "src/index.ts", 999); + expect(result).toBeUndefined(); + }); + + test("should return undefined when path doesn't match", () => { + const result = findExistingRootComment( + mockComments, + "src/nonexistent.ts", + 42, + ); + expect(result).toBeUndefined(); + }); + + test("should return undefined when targetLine is undefined", () => { + const result = findExistingRootComment( + mockComments, + "src/index.ts", + undefined, + ); + expect(result).toBeUndefined(); + }); + + test("should return undefined when comments array is empty", () => { + const result = findExistingRootComment([], "src/index.ts", 42); + expect(result).toBeUndefined(); + }); + }); + + describe("resolveInReplyTo", () => { + const mockComments: ReviewComment[] = [ + { + id: 100, + path: "src/index.ts", + line: 42, + original_line: 42, + in_reply_to_id: null, + }, + { + id: 102, + path: "src/utils.ts", + line: 10, + original_line: 10, + in_reply_to_id: null, + }, + ]; + + test("should return explicit in_reply_to when provided", () => { + const result = resolveInReplyTo(999, mockComments, "src/index.ts", 42); + expect(result).toBe(999); + }); + + test("should auto-detect existing comment when in_reply_to is undefined", () => { + const result = resolveInReplyTo( + undefined, + mockComments, + "src/index.ts", + 42, + ); + expect(result).toBe(100); + }); + + test("should return undefined when no explicit in_reply_to and no existing comment", () => { + const result = resolveInReplyTo( + undefined, + mockComments, + "src/index.ts", + 999, + ); + expect(result).toBeUndefined(); + }); + + test("should prioritize explicit in_reply_to over auto-detection", () => { + // Even though there's an existing comment on line 42, explicit value takes precedence + const result = resolveInReplyTo(555, mockComments, "src/index.ts", 42); + expect(result).toBe(555); + }); + + test("should handle empty comments array with undefined in_reply_to", () => { + const result = resolveInReplyTo(undefined, [], "src/index.ts", 42); + expect(result).toBeUndefined(); + }); + }); + + describe("auto-deduplication scenarios", () => { + test("Scenario: First review creates new comment (no existing comments)", () => { + const existingComments: ReviewComment[] = []; + const result = resolveInReplyTo( + undefined, + existingComments, + "src/index.ts", + 42, + ); + // No existing comment, should create new thread + expect(result).toBeUndefined(); + }); + + test("Scenario: Second review should reply to first comment (deduplication)", () => { + const existingComments: ReviewComment[] = [ + { + id: 100, + path: "src/index.ts", + line: 42, + original_line: 42, + in_reply_to_id: null, + }, + ]; + const result = resolveInReplyTo( + undefined, + existingComments, + "src/index.ts", + 42, + ); + // Should reply to existing comment instead of creating duplicate + expect(result).toBe(100); + }); + + test("Scenario: Third review should still reply to root comment (not to second reply)", () => { + const existingComments: ReviewComment[] = [ + { + id: 100, + path: "src/index.ts", + line: 42, + original_line: 42, + in_reply_to_id: null, // Root comment + }, + { + id: 101, + path: "src/index.ts", + line: 42, + original_line: 42, + in_reply_to_id: 100, // Reply to root + }, + ]; + const result = resolveInReplyTo( + undefined, + existingComments, + "src/index.ts", + 42, + ); + // Should reply to root comment (100), not to the reply (101) + expect(result).toBe(100); + }); + + test("Scenario: Different file should not trigger deduplication", () => { + const existingComments: ReviewComment[] = [ + { + id: 100, + path: "src/index.ts", + line: 42, + original_line: 42, + in_reply_to_id: null, + }, + ]; + const result = resolveInReplyTo( + undefined, + existingComments, + "src/other.ts", // Different file + 42, + ); + // Different file, should create new thread + expect(result).toBeUndefined(); + }); + + test("Scenario: Different line should not trigger deduplication", () => { + const existingComments: ReviewComment[] = [ + { + id: 100, + path: "src/index.ts", + line: 42, + original_line: 42, + in_reply_to_id: null, + }, + ]; + const result = resolveInReplyTo( + undefined, + existingComments, + "src/index.ts", + 50, // Different line + ); + // Different line, should create new thread + expect(result).toBeUndefined(); + }); + }); +}); diff --git a/src/create-prompt/index.ts b/src/create-prompt/index.ts index 66149eac3..3b295407d 100644 --- a/src/create-prompt/index.ts +++ b/src/create-prompt/index.ts @@ -555,7 +555,13 @@ ${sanitizeContent(eventData.commentBody)} Your request is in above${eventData.eventName === "issues" ? ` (or the ${entityType} body for assigned/labeled events)` : ""}. Decide what's being asked: -1. **Question or code review** - Answer directly or provide feedback +1. **Question or code review** - Answer directly or provide feedback${ + eventData.isPR + ? ` + - For inline comments: Use mcp__github_inline_comment__create_inline_comment to add specific feedback on lines + - Duplicate comments are automatically prevented - if a comment exists on a line, yours will be added as a reply` + : "" + } 2. **Code change** - Implement the change, commit, and push Communication: @@ -735,7 +741,15 @@ ${eventData.eventName === "issue_comment" || eventData.eventName === "pull_reque - 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_comment__update_claude_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_comment__update_claude_comment to post your review + - INLINE COMMENTS (PR reviews): + - You have access to mcp__github_inline_comment__create_inline_comment to add inline comments on specific lines + - The tool automatically prevents duplicate comments - if a comment already exists on a line, your comment will be added as a reply to the existing thread + - If you see existing comments in the section with comment IDs, you can reference them in your feedback` + : "" + } - 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.${ diff --git a/src/mcp/github-inline-comment-server.ts b/src/mcp/github-inline-comment-server.ts index 703cda2e0..4d780a724 100644 --- a/src/mcp/github-inline-comment-server.ts +++ b/src/mcp/github-inline-comment-server.ts @@ -4,6 +4,7 @@ import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js" import { z } from "zod"; import { createOctokit } from "../github/api/client"; import { sanitizeContent } from "../github/utils/sanitizer"; +import { resolveInReplyTo } from "./inline-comment-utils"; // Get repository and PR information from environment variables const REPO_OWNER = process.env.REPO_OWNER; @@ -67,8 +68,14 @@ server.tool( .describe( "Specific commit SHA to comment on (defaults to latest commit)", ), + in_reply_to: z + .number() + .optional() + .describe( + "Comment ID to reply to (creates a reply in an existing thread instead of a new thread)", + ), }, - async ({ path, body, line, startLine, side, commit_id }) => { + async ({ path, body, line, startLine, side, commit_id, in_reply_to }) => { try { const githubToken = process.env.GITHUB_TOKEN; @@ -124,6 +131,32 @@ server.tool( params.line = line; } + // Resolve in_reply_to: use explicit value or auto-detect from existing comments + const { data: existingComments } = + await octokit.rest.pulls.listReviewComments({ + owner, + repo, + pull_number, + per_page: 100, + }); + + const resolvedInReplyTo = resolveInReplyTo( + in_reply_to, + existingComments, + path, + line, + ); + + if (resolvedInReplyTo !== undefined) { + params.in_reply_to = resolvedInReplyTo; + if (in_reply_to === undefined) { + // Auto-deduplication case + console.log( + `Auto-deduplication: Found existing comment ${resolvedInReplyTo} on ${path}:${line}. Replying to existing thread instead of creating duplicate.`, + ); + } + } + const result = await octokit.rest.pulls.createReviewComment(params); return { diff --git a/src/mcp/inline-comment-utils.ts b/src/mcp/inline-comment-utils.ts new file mode 100644 index 000000000..7ead15f45 --- /dev/null +++ b/src/mcp/inline-comment-utils.ts @@ -0,0 +1,69 @@ +/** + * Utility functions for inline comment handling + * Extracted for testability + */ + +export interface ReviewComment { + id: number; + path: string; + line?: number | null; + original_line?: number | null; + in_reply_to_id?: number | null; +} + +/** + * Find an existing root comment on the same line and path + * Used for auto-deduplication to reply to existing threads instead of creating duplicates + * + * @param existingComments - List of existing review comments on the PR + * @param targetPath - The file path to check + * @param targetLine - The line number to check + * @returns The existing root comment if found, otherwise undefined + */ +export function findExistingRootComment( + existingComments: ReviewComment[], + targetPath: string, + targetLine: number | undefined, +): ReviewComment | undefined { + if (targetLine === undefined) { + return undefined; + } + + return existingComments.find( + (comment) => + comment.path === targetPath && + (comment.line === targetLine || comment.original_line === targetLine) && + !comment.in_reply_to_id, + ); +} + +/** + * Determine if we should reply to an existing comment + * Returns the comment ID to reply to, or undefined to create a new thread + * + * @param explicitInReplyTo - Explicitly specified comment ID to reply to + * @param existingComments - List of existing review comments on the PR + * @param targetPath - The file path to check + * @param targetLine - The line number to check + * @returns Comment ID to reply to, or undefined + */ +export function resolveInReplyTo( + explicitInReplyTo: number | undefined, + existingComments: ReviewComment[], + targetPath: string, + targetLine: number | undefined, +): number | undefined { + // If explicitly specified, use that + if (explicitInReplyTo !== undefined) { + return explicitInReplyTo; + } + + // Otherwise, try to find an existing comment on the same line + const existingComment = findExistingRootComment( + existingComments, + targetPath, + targetLine, + ); + + return existingComment?.id; +}