diff --git a/entry b/entry index f0236bb..df4bda3 100755 --- a/entry +++ b/entry @@ -106637,7 +106637,7 @@ var handleToolError = (error50) => { }; }; var execute = (fn2, toolName) => { - return async (params) => { + const _fn = async (params) => { try { const result = await fn2(params); return handleToolSuccess(result); @@ -106649,6 +106649,8 @@ var execute = (fn2, toolName) => { return handleToolError(error50); } }; + _fn.raw = fn2; + return _fn; }; function sanitizeSchema(schema2) { if (!schema2 || typeof schema2 !== "object") { @@ -136777,13 +136779,24 @@ function $(cmd, args3, options) { // mcp/checkout.ts function formatFilesWithLineNumbers(files) { const output = []; + const tocEntries = []; + const tocHeaderSize = 1 + files.length + 2; + let currentLine = tocHeaderSize + 1; for (const file2 of files) { + const fileStartLine = currentLine; output.push(`diff --git a/${file2.filename} b/${file2.filename}`); output.push(`--- a/${file2.filename}`); output.push(`+++ b/${file2.filename}`); + currentLine += 3; if (!file2.patch) { output.push("(binary file or no changes)"); output.push(""); + currentLine += 2; + tocEntries.push({ + filename: file2.filename, + startLine: fileStartLine, + endLine: currentLine - 1 + }); continue; } const lines = file2.patch.split("\n"); @@ -136795,6 +136808,7 @@ function formatFilesWithLineNumbers(files) { oldLine = parseInt(hunkMatch[1], 10); newLine = parseInt(hunkMatch[2], 10); output.push(line); + currentLine++; continue; } const changeType = line[0] || " "; @@ -136816,10 +136830,26 @@ function formatFilesWithLineNumbers(files) { } else { output.push(line); } + currentLine++; } output.push(""); + currentLine++; + tocEntries.push({ + filename: file2.filename, + startLine: fileStartLine, + endLine: currentLine - 1 + }); } - return output.join("\n"); + const tocLines = [`## Files (${files.length})`]; + for (const entry of tocEntries) { + tocLines.push(`- ${entry.filename} \u2192 lines ${entry.startLine}-${entry.endLine}`); + } + tocLines.push(""); + tocLines.push("---"); + tocLines.push(""); + const toc = tocLines.join("\n"); + const content = toc + output.join("\n"); + return { content, toc }; } function padNum(n) { return n.toString().padStart(4, " "); @@ -136827,6 +136857,15 @@ function padNum(n) { var CheckoutPr = type({ pull_number: type.number.describe("the pull request number to checkout") }); +async function fetchAndFormatPrDiff(params) { + const filesResponse = await params.octokit.rest.pulls.listFiles({ + owner: params.owner, + repo: params.repo, + pull_number: params.pullNumber, + per_page: 100 + }); + return formatFilesWithLineNumbers(filesResponse.data); +} async function checkoutPrBranch(params) { const { octokit, owner, name, token, pullNumber } = params; log.info(`\xBB checking out PR #${pullNumber}...`); @@ -136907,14 +136946,13 @@ function CheckoutPrTool(ctx) { if (!headRepo) { throw new Error(`PR #${pull_number} source repository was deleted`); } - const filesResponse = await ctx.octokit.rest.pulls.listFiles({ + const formatResult = await fetchAndFormatPrDiff({ + octokit: ctx.octokit, owner: ctx.repo.owner, repo: ctx.repo.name, - pull_number, - per_page: 100 + pullNumber: pull_number }); - const diffContent = formatFilesWithLineNumbers(filesResponse.data); - const diffPreview = diffContent.split("\n").slice(0, 100).join("\n"); + const diffPreview = formatResult.content.split("\n").slice(0, 100).join("\n"); log.debug(`formatted diff preview (first 100 lines): ${diffPreview}`); const tempDir = process.env.PULLFROG_TEMP_DIR; @@ -136924,8 +136962,8 @@ ${diffPreview}`); ); } const diffPath = join2(tempDir, `pr-${pull_number}.diff`); - writeFileSync2(diffPath, diffContent); - log.debug(`wrote diff to ${diffPath} (${diffContent.length} bytes)`); + writeFileSync2(diffPath, formatResult.content); + log.debug(`wrote diff to ${diffPath} (${formatResult.content.length} bytes)`); return { success: true, number: pr.data.number, @@ -138486,7 +138524,7 @@ function CommitInfoTool(ctx) { }); const data = response.data; const files = data.files ?? []; - const diffContent = formatFilesWithLineNumbers(files); + const formatResult = formatFilesWithLineNumbers(files); const tempDir = process.env.PULLFROG_TEMP_DIR; if (!tempDir) { throw new Error( @@ -138494,8 +138532,8 @@ function CommitInfoTool(ctx) { ); } const diffFile = join5(tempDir, `commit-${sha.slice(0, 7)}.diff`); - writeFileSync3(diffFile, diffContent); - log.debug(`wrote commit diff to ${diffFile} (${diffContent.length} bytes)`); + writeFileSync3(diffFile, formatResult.content); + log.debug(`wrote commit diff to ${diffFile} (${formatResult.content.length} bytes)`); return { sha: data.sha, message: data.commit.message, @@ -138688,56 +138726,41 @@ function CreatePullRequestReviewTool(ctx) { // mcp/reviewComments.ts import { writeFileSync as writeFileSync4 } from "node:fs"; import { join as join6 } from "node:path"; -var REPLY_TO_FRAGMENT = ` - replyTo { - databaseId - body - author { login } - replyTo { - databaseId - body - author { login } - replyTo { - databaseId - body - author { login } - replyTo { - databaseId - body - author { login } - replyTo { - databaseId - body - author { login } - } - } - } - } - } -`; -var REVIEW_QUERY = ` -query ($nodeId: ID!) { - node(id: $nodeId) { - ... on PullRequestReview { - databaseId - author { login } - comments(first: 100) { +var REVIEW_THREADS_QUERY = ` +query ($owner: String!, $name: String!, $prNumber: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $prNumber) { + reviewThreads(first: 100) { nodes { - databaseId - body + id path line startLine - diffHunk - url - author { login } - createdAt - ${REPLY_TO_FRAGMENT} - reactionGroups { - content - reactors(first: 10) { - nodes { - ... on Actor { login } + diffSide + isResolved + isOutdated + comments(first: 50) { + nodes { + fullDatabaseId + body + createdAt + diffHunk + line + startLine + originalLine + originalStartLine + author { login } + pullRequestReview { + databaseId + author { login } + } + reactionGroups { + content + reactors(first: 10) { + nodes { + ... on Actor { login } + } + } } } } @@ -138747,123 +138770,306 @@ query ($nodeId: ID!) { } } `; -var MAX_BODY_PREVIEW = 80; -function truncateBody(body) { - const oneLine = body.replace(/\n/g, " ").trim(); - if (oneLine.length <= MAX_BODY_PREVIEW) return oneLine; - return oneLine.slice(0, MAX_BODY_PREVIEW - 3) + "..."; +var CONTEXT_PADDING = 3; +function extractCommentedLines(diffHunk, startLine, endLine, side) { + const lines = diffHunk.split("\n"); + if (lines.length <= 1) return diffHunk; + const header = lines[0]; + const contentLines = lines.slice(1); + const headerMatch = header.match(/@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/); + if (!headerMatch) return diffHunk; + const hunkOldStart = parseInt(headerMatch[1], 10); + const hunkNewStart = parseInt(headerMatch[2], 10); + const hunkStart = side === "LEFT" ? hunkOldStart : hunkNewStart; + const commentStart = startLine ?? endLine ?? hunkStart; + const commentEnd = endLine ?? commentStart; + const diffLines = []; + let oldLineNum = hunkOldStart; + let newLineNum = hunkNewStart; + for (const line of contentLines) { + const prefix = line[0]; + if (prefix === "-") { + diffLines.push({ text: line, lineNum: side === "LEFT" ? oldLineNum : null }); + oldLineNum++; + } else if (prefix === "+") { + diffLines.push({ text: line, lineNum: side === "RIGHT" ? newLineNum : null }); + newLineNum++; + } else { + diffLines.push({ text: line, lineNum: side === "LEFT" ? oldLineNum : newLineNum }); + oldLineNum++; + newLineNum++; + } + } + const targetStart = commentStart - CONTEXT_PADDING; + const targetEnd = commentEnd; + const result = []; + let truncatedBefore = 0; + for (let i = 0; i < diffLines.length; i++) { + const dl = diffLines[i]; + const inRange = dl.lineNum !== null && dl.lineNum >= targetStart && dl.lineNum <= targetEnd; + const adjacentOtherSide = dl.lineNum === null && result.length > 0 && i < diffLines.length - 1; + if (inRange || adjacentOtherSide) { + result.push(dl.text); + } else if (result.length === 0) { + truncatedBefore++; + } + } + if (truncatedBefore > 0) { + return `${header} +... (${truncatedBefore} lines above) ... +${result.join("\n")}`; + } + return `${header} +${result.join("\n")}`; } -function escapeXml(text) { - return text.replace(/&/g, "&").replace(//g, ">").replace(/"/g, """); +function parseFilePatches(patch) { + const hunks = []; + const lines = patch.split("\n"); + let currentHunk = null; + for (const line of lines) { + const hunkMatch = line.match(/@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/); + if (hunkMatch) { + if (currentHunk) hunks.push(currentHunk); + currentHunk = { + header: line, + oldStart: parseInt(hunkMatch[1], 10), + oldCount: parseInt(hunkMatch[2] ?? "1", 10), + newStart: parseInt(hunkMatch[3], 10), + newCount: parseInt(hunkMatch[4] ?? "1", 10), + content: [] + }; + } else if (currentHunk) { + currentHunk.content.push(line); + } + } + if (currentHunk) hunks.push(currentHunk); + return hunks; } +function findOverlappingHunks(hunks, startLine, endLine, side) { + return hunks.filter((hunk) => { + const hunkStart = side === "LEFT" ? hunk.oldStart : hunk.newStart; + const hunkCount = side === "LEFT" ? hunk.oldCount : hunk.newCount; + const hunkEnd = hunkStart + hunkCount - 1; + return startLine <= hunkEnd && hunkStart <= endLine; + }); +} +function extractFromFilePatches(hunks, startLine, endLine, side) { + const overlapping = findOverlappingHunks(hunks, startLine, endLine, side); + if (overlapping.length === 0) { + return `(no diff hunks found for lines ${startLine}-${endLine})`; + } + if (overlapping.length === 1) { + const hunk = overlapping[0]; + const fullHunk = hunk.header + "\n" + hunk.content.join("\n"); + return extractCommentedLines(fullHunk, startLine, endLine, side); + } + const result = []; + let prevHunkEnd = 0; + for (let i = 0; i < overlapping.length; i++) { + const hunk = overlapping[i]; + const hunkStart = side === "LEFT" ? hunk.oldStart : hunk.newStart; + const hunkCount = side === "LEFT" ? hunk.oldCount : hunk.newCount; + const hunkEnd = hunkStart + hunkCount - 1; + if (i > 0 && hunkStart > prevHunkEnd + 1) { + const gapSize = hunkStart - prevHunkEnd - 1; + result.push(` +... (${gapSize} unchanged lines) ... +`); + } + result.push(hunk.header); + result.push(...hunk.content); + prevHunkEnd = hunkEnd; + } + return result.join("\n"); +} +var GetReviewComments = type({ + pull_number: type.number.describe("The pull request number"), + review_id: type.number.describe("The review ID to get comments for"), + approved_by: type.string.describe( + "Optional GitHub username - only return threads where this user gave a \u{1F44D} to at least one comment" + ).optional() +}); function hasThumbsUpFrom(comment, username) { if (!comment.reactionGroups) return false; const thumbsUp = comment.reactionGroups.find((g) => g.content === "THUMBS_UP"); if (!thumbsUp?.reactors?.nodes) return false; return thumbsUp.reactors.nodes.some((r) => r?.login === username); } -function flattenReplyToChain(replyTo) { - if (!replyTo) return []; - const parent = flattenReplyToChain(replyTo.replyTo ?? null); - return [...parent, { body: replyTo.body, author: replyTo.author?.login ?? "unknown" }]; +function threadHasThumbsUpFrom(thread, username) { + const comments = thread.comments?.nodes ?? []; + return comments.some((c) => c && hasThumbsUpFrom(c, username)); +} +function formatReviewThreads(threadBlocks, header) { + const tocHeaderLines = 4; + const tocFooterLines = 3; + let currentLine = tocHeaderLines + threadBlocks.length + tocFooterLines + 1; + const tocEntries = []; + const threadLines = []; + for (const block of threadBlocks) { + const startLine = currentLine; + const actualLineCount = block.content.reduce((sum, line) => sum + line.split("\n").length, 0); + const endLine = currentLine + actualLineCount - 1; + tocEntries.push(`- ${block.path}:${block.lineRange} \u2192 lines ${startLine}-${endLine}`); + threadLines.push(...block.content); + currentLine += actualLineCount; + } + const lines = []; + lines.push( + `# Review Threads (${threadBlocks.length}) for PR #${header.pullNumber} - Review ${header.reviewId} by ${header.reviewer}` + ); + lines.push(""); + lines.push("## TOC"); + lines.push(""); + lines.push(...tocEntries); + lines.push(""); + lines.push("---"); + lines.push(""); + lines.push(...threadLines); + return { + toc: tocEntries.join("\n"), + content: lines.join("\n") + }; +} +function buildThreadBlocks(threads, filePatchMap, reviewId) { + const firstMatchingComment = threads[0]?.comments?.nodes?.find( + (c) => c?.pullRequestReview?.databaseId === reviewId + ); + const reviewer = firstMatchingComment?.pullRequestReview?.author?.login ?? "unknown"; + threads.sort((a, b) => { + const pathCmp = a.path.localeCompare(b.path); + if (pathCmp !== 0) return pathCmp; + const aLine = a.startLine ?? a.line ?? 0; + const bLine = b.startLine ?? b.line ?? 0; + return aLine - bLine; + }); + const threadBlocks = []; + for (const thread of threads) { + const allComments = (thread.comments?.nodes ?? []).filter( + (c) => c !== null + ); + if (allComments.length === 0) continue; + const firstComment = allComments[0]; + const line = thread.line ?? firstComment?.line ?? firstComment?.originalLine ?? thread.startLine ?? 0; + const startLine = thread.startLine ?? firstComment?.startLine ?? firstComment?.originalStartLine ?? line; + const lineRange = startLine === line ? `${line}` : `${startLine}-${line}`; + const block = []; + const status = thread.isResolved ? " [RESOLVED]" : thread.isOutdated ? " [OUTDATED]" : ""; + block.push(`## ${thread.path}:${lineRange}${status}`); + block.push(""); + for (const comment of allComments) { + const author = comment.author?.login ?? "unknown"; + const isTargetReview = comment.pullRequestReview?.databaseId === reviewId; + const marker = isTargetReview ? " *" : ""; + block.push( + `\`\`\`\`comment author=${author} id=${comment.fullDatabaseId ?? "unknown"} review=${comment.pullRequestReview?.databaseId ?? "unknown"}${marker}` + ); + block.push(comment.body || "(no comment body)"); + block.push("````"); + block.push(""); + } + const fileHunks = filePatchMap.get(thread.path); + const firstCommentWithHunk = allComments.find((c) => c.diffHunk); + let diffContent = null; + if (fileHunks && fileHunks.length > 0) { + const overlapping = findOverlappingHunks(fileHunks, startLine, line, thread.diffSide); + if (overlapping.length > 0) { + diffContent = extractFromFilePatches(fileHunks, startLine, line, thread.diffSide); + } + } + if (!diffContent && firstCommentWithHunk) { + diffContent = extractCommentedLines( + firstCommentWithHunk.diffHunk, + startLine, + line, + thread.diffSide + ); + } + if (diffContent) { + block.push(`\`\`\`diff file=${thread.path} lines=${lineRange} side=${thread.diffSide}`); + block.push(diffContent); + block.push("```"); + block.push(""); + } else { + block.push(`\`\`\`diff file=${thread.path} lines=${lineRange} side=${thread.diffSide}`); + block.push(`(no diff context available - comment on unchanged lines)`); + block.push("```"); + block.push(""); + } + threadBlocks.push({ path: thread.path, lineRange, content: block }); + } + return { threadBlocks, reviewer }; } -var GetReviewComments = type({ - pull_number: type.number.describe("The pull request number"), - review_id: type.number.describe("The review ID to get comments for"), - approved_by: type.string.describe("Optional GitHub username - only return comments this user gave a \u{1F44D} to").optional() -}); function GetReviewCommentsTool(ctx) { return tool({ name: "get_review_comments", - description: "Get review comments for a pull request review, including thread context. When approved_by is provided, only returns comments that user approved with \u{1F44D}. Returns commentsPath pointing to a file with full comment details in XML format.", + description: "Get review comments for a pull request review with full thread context. When approved_by is provided, only returns threads where that user gave a \u{1F44D} to at least one comment. Returns a TOC and commentsPath pointing to a markdown file with full comment details.", parameters: GetReviewComments, - execute: execute(async ({ pull_number, review_id, approved_by }) => { - const { data: review } = await ctx.octokit.rest.pulls.getReview({ + execute: execute(async (params) => { + const response = await ctx.octokit.graphql(REVIEW_THREADS_QUERY, { + owner: ctx.repo.owner, + name: ctx.repo.name, + prNumber: params.pull_number + }); + const allThreads = response.repository?.pullRequest?.reviewThreads?.nodes ?? []; + let threadsForReview = allThreads.filter((thread) => { + if (!thread?.comments?.nodes) return false; + return thread.comments.nodes.some( + (c) => c?.pullRequestReview?.databaseId === params.review_id + ); + }); + if (params.approved_by) { + threadsForReview = threadsForReview.filter( + (thread) => threadHasThumbsUpFrom(thread, params.approved_by) + ); + } + if (threadsForReview.length === 0) { + return { + review_id: params.review_id, + pull_number: params.pull_number, + reviewer: "unknown", + threadCount: 0, + commentsPath: null, + toc: null, + instructions: params.approved_by ? `no threads with \u{1F44D} from ${params.approved_by}` : "no threads found for this review" + }; + } + const prFilesResponse = await ctx.octokit.rest.pulls.listFiles({ owner: ctx.repo.owner, repo: ctx.repo.name, - pull_number, - review_id + pull_number: params.pull_number }); - const reviewer = review.user?.login ?? "unknown"; - const response = await ctx.octokit.graphql(REVIEW_QUERY, { - nodeId: review.node_id - }); - const reviewComments = response.node?.comments?.nodes; - if (!reviewComments) { - return { - review_id, - pull_number, - reviewer, - count: 0, - commentsPath: null, - message: "No comments found for this review" - }; - } - const allComments = reviewComments.filter((c) => c !== null); - const comments = approved_by ? allComments.filter((c) => hasThumbsUpFrom(c, approved_by)) : allComments; - if (comments.length === 0) { - return { - review_id, - pull_number, - reviewer, - count: 0, - commentsPath: null, - message: approved_by ? `No comments with \u{1F44D} from ${approved_by}` : "No comments found for this review" - }; - } - const lines = []; - lines.push(``); - lines.push(""); - lines.push(""); - for (const comment of comments) { - const line = comment.line ?? comment.startLine ?? 0; - const preview = escapeXml(truncateBody(comment.body)); - lines.push( - ` ${preview}` - ); - } - lines.push(""); - lines.push(""); - for (const comment of comments) { - const line = comment.line ?? comment.startLine ?? 0; - const author = comment.author?.login ?? "unknown"; - lines.push( - `` - ); - const thread = flattenReplyToChain(comment.replyTo); - if (thread.length > 0) { - lines.push(" "); - for (const msg of thread) { - lines.push( - ` ${escapeXml(msg.body)}` - ); - } - lines.push(" "); + const filePatchMap = /* @__PURE__ */ new Map(); + for (const file2 of prFilesResponse.data) { + if (file2.patch) { + filePatchMap.set(file2.filename, parseFilePatches(file2.patch)); } - lines.push(" "); - lines.push(escapeXml(comment.diffHunk)); - lines.push(" "); - lines.push(` ${escapeXml(comment.body)}`); - lines.push(""); - lines.push(""); } - lines.push(""); - const content = lines.join("\n"); + const { threadBlocks, reviewer } = buildThreadBlocks( + threadsForReview, + filePatchMap, + params.review_id + ); + const formatted = formatReviewThreads(threadBlocks, { + pullNumber: params.pull_number, + reviewId: params.review_id, + reviewer + }); const tempDir = process.env.PULLFROG_TEMP_DIR; if (!tempDir) { throw new Error("PULLFROG_TEMP_DIR not set"); } - const filename = approved_by ? `review-${review_id}-approved-by-${approved_by}.xml` : `review-${review_id}-comments.xml`; + const filename = `review-${params.review_id}-threads.md`; const commentsPath = join6(tempDir, filename); - writeFileSync4(commentsPath, content); - log.info(`wrote ${comments.length} comments to ${commentsPath}`); - log.box(content); + writeFileSync4(commentsPath, formatted.content); + log.debug(`wrote ${threadBlocks.length} threads to ${commentsPath}`); return { - review_id, - pull_number, + review_id: params.review_id, + pull_number: params.pull_number, reviewer, - count: comments.length, - commentsPath + threadCount: threadBlocks.length, + commentsPath, + toc: formatted.toc, + instructions: `the file at commentsPath contains ${threadBlocks.length} review threads with full conversation history. comments marked with * are from the target review (${params.review_id}). the TOC shows each thread's file:line and the line number where it appears in the file. to read a specific thread, use: grep -A 50 "^## " ${commentsPath} (replace with the path from the TOC, e.g. "^## action/utils/foo.ts:42"). address each thread in order, working through one file at a time.` }; }) }); @@ -138876,14 +139082,14 @@ function ListPullRequestReviewsTool(ctx) { name: "list_pull_request_reviews", description: "List all reviews for a pull request. Returns all reviews including approvals, request changes, and comments.", parameters: ListPullRequestReviews, - execute: execute(async ({ pull_number }) => { + execute: execute(async (params) => { const reviews = await ctx.octokit.paginate(ctx.octokit.rest.pulls.listReviews, { owner: ctx.repo.owner, repo: ctx.repo.name, - pull_number + pull_number: params.pull_number }); return { - pull_number, + pull_number: params.pull_number, reviews: reviews.map((review) => ({ id: review.id, node_id: review.node_id, diff --git a/mcp/__snapshots__/checkout.test.ts.snap b/mcp/__snapshots__/checkout.test.ts.snap new file mode 100644 index 0000000..02751a1 --- /dev/null +++ b/mcp/__snapshots__/checkout.test.ts.snap @@ -0,0 +1,146 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[`fetchAndFormatPrDiff > fetches PR files and generates TOC with formatted diff > content 1`] = ` +"## Files (3) +- .github/workflows/test.yml → lines 7-47 +- index.test.ts → lines 48-110 +- index.ts → lines 111-132 + +--- +diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml +--- a/.github/workflows/test.yml ++++ b/.github/workflows/test.yml +@@ -0,0 +1,36 @@ +| | 1 | + | name: Test +| | 2 | + | +| | 3 | + | on: +| | 4 | + | push: +| | 5 | + | branches: [main] +| | 6 | + | pull_request: +| | 7 | + | branches: [main] +| | 8 | + | +| | 9 | + | jobs: +| | 10 | + | test: +| | 11 | + | runs-on: ubuntu-latest +| | 12 | + | +| | 13 | + | strategy: +| | 14 | + | matrix: +| | 15 | + | node-version: [22.x] +| | 16 | + | +| | 17 | + | steps: +| | 18 | + | - name: Checkout code +| | 19 | + | uses: actions/checkout@v4 +| | 20 | + | +| | 21 | + | - name: Setup pnpm +| | 22 | + | uses: pnpm/action-setup@v2 +| | 23 | + | with: +| | 24 | + | version: 8 +| | 25 | + | +| | 26 | + | - name: Setup Node.js \${{ matrix.node-version }} +| | 27 | + | uses: actions/setup-node@v4 +| | 28 | + | with: +| | 29 | + | node-version: \${{ matrix.node-version }} +| | 30 | + | cache: 'pnpm' +| | 31 | + | +| | 32 | + | - name: Install dependencies +| | 33 | + | run: pnpm install +| | 34 | + | +| | 35 | + | - name: Run tests +| | 36 | + | run: pnpm test + +diff --git a/index.test.ts b/index.test.ts +--- a/index.test.ts ++++ b/index.test.ts +@@ -1,5 +1,5 @@ +| 1 | 1 | | import { describe, it, expect } from 'vitest' +| 2 | | - | import { add } from './index.js' +| | 2 | + | import { add, multiply, subtract, divide } from './index.js' +| 3 | 3 | | +| 4 | 4 | | describe('add function', () => { +| 5 | 5 | | it('should add two positive numbers correctly', () => { +@@ -25,3 +25,51 @@ describe('add function', () => { +| 25 | 25 | | expect(add(0.1, 0.2)).toBeCloseTo(0.3) +| 26 | 26 | | }) +| 27 | 27 | | }) +| | 28 | + | +| | 29 | + | describe('multiply function', () => { +| | 30 | + | it('should multiply two positive numbers correctly', () => { +| | 31 | + | expect(multiply(3, 4)).toBe(12) +| | 32 | + | }) +| | 33 | + | +| | 34 | + | it('should multiply negative numbers correctly', () => { +| | 35 | + | expect(multiply(-2, 3)).toBe(-6) +| | 36 | + | expect(multiply(-2, -3)).toBe(6) +| | 37 | + | }) +| | 38 | + | +| | 39 | + | it('should handle zero correctly', () => { +| | 40 | + | expect(multiply(5, 0)).toBe(0) +| | 41 | + | expect(multiply(0, 5)).toBe(0) +| | 42 | + | }) +| | 43 | + | }) +| | 44 | + | +| | 45 | + | describe('subtract function', () => { +| | 46 | + | it('should subtract two positive numbers correctly', () => { +| | 47 | + | expect(subtract(10, 3)).toBe(7) +| | 48 | + | }) +| | 49 | + | +| | 50 | + | it('should handle negative numbers correctly', () => { +| | 51 | + | expect(subtract(5, -3)).toBe(8) +| | 52 | + | expect(subtract(-5, 3)).toBe(-8) +| | 53 | + | }) +| | 54 | + | +| | 55 | + | it('should handle zero correctly', () => { +| | 56 | + | expect(subtract(5, 0)).toBe(5) +| | 57 | + | expect(subtract(0, 5)).toBe(-5) +| | 58 | + | }) +| | 59 | + | }) +| | 60 | + | +| | 61 | + | describe('divide function', () => { +| | 62 | + | it('should divide two positive numbers correctly', () => { +| | 63 | + | expect(divide(10, 2)).toBe(5) +| | 64 | + | }) +| | 65 | + | +| | 66 | + | it('should handle negative numbers correctly', () => { +| | 67 | + | expect(divide(-10, 2)).toBe(-5) +| | 68 | + | expect(divide(10, -2)).toBe(-5) +| | 69 | + | }) +| | 70 | + | +| | 71 | + | it('should handle decimal results correctly', () => { +| | 72 | + | expect(divide(10, 3)).toBeCloseTo(3.333, 2) +| | 73 | + | expect(divide(7, 2)).toBe(3.5) +| | 74 | + | }) +| | 75 | + | }) + +diff --git a/index.ts b/index.ts +--- a/index.ts ++++ b/index.ts +@@ -3,11 +3,13 @@ export function add(a: number, b: number) { +| 3 | 3 | | } +| 4 | 4 | | +| 5 | 5 | | export function multiply(a: number, b: number) { +| 6 | | - | // Bug: accidentally adding 1 to the result +| 7 | | - | return a * b + 1; +| | 6 | + | return a * b; +| 8 | 7 | | } +| 9 | 8 | | +| 10 | 9 | | export function subtract(a: number, b: number) { +| 11 | | - | // Bug: accidentally adding instead of subtracting +| 12 | | - | return a + b; +| | 10 | + | return a - b; +| | 11 | + | } +| | 12 | + | +| | 13 | + | export function divide(a: number, b: number) { +| | 14 | + | return a / b; +| 13 | 15 | | } +" +`; + +exports[`fetchAndFormatPrDiff > fetches PR files and generates TOC with formatted diff > toc 1`] = ` +"## Files (3) +- .github/workflows/test.yml → lines 7-47 +- index.test.ts → lines 48-110 +- index.ts → lines 111-132 + +--- +" +`; diff --git a/mcp/__snapshots__/reviewComments.test.ts.snap b/mcp/__snapshots__/reviewComments.test.ts.snap new file mode 100644 index 0000000..bb683f5 --- /dev/null +++ b/mcp/__snapshots__/reviewComments.test.ts.snap @@ -0,0 +1,42 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[`formatReviewThreads > formats thread blocks with TOC and correct line numbers > content 1`] = ` +"# Review Threads (1) for PR #49 - Review 3485940013 by cursor + +## TOC + +- .github/workflows/test.yml:7 → lines 9-36 + +--- + +## .github/workflows/test.yml:7 [RESOLVED] + +\`\`\`\`comment author=cursor id=2544544046 review=3485940013 * +### Bug: GitHub Actions workflow triggered for wrong branch + + + + +The \`pull_request\` trigger specifies \`branches: [mainc]\`, but the \`push\` trigger specifies \`branches: [main]\`. This mismatch means pull requests will only trigger tests if targeting a non-existent \`mainc\` branch rather than the actual \`main\` development branch, preventing CI from running on most pull requests. + + + +Fix in Cursor Fix in Web + + +\`\`\`\` + +\`\`\`diff file=.github/workflows/test.yml lines=7 side=RIGHT +@@ -0,0 +1,36 @@ +... (3 lines above) ... ++ push: ++ branches: [main] ++ pull_request: ++ branches: [main] +\`\`\` +" +`; + +exports[`formatReviewThreads > formats thread blocks with TOC and correct line numbers > toc 1`] = `"- .github/workflows/test.yml:7 → lines 9-36"`; diff --git a/mcp/checkout.test.ts b/mcp/checkout.test.ts new file mode 100644 index 0000000..c9fc862 --- /dev/null +++ b/mcp/checkout.test.ts @@ -0,0 +1,36 @@ +import { Octokit } from "@octokit/rest"; +import { describe, expect, it } from "vitest"; +import { fetchAndFormatPrDiff } from "./checkout.ts"; + +describe("fetchAndFormatPrDiff", () => { + it("fetches PR files and generates TOC with formatted diff", async () => { + const token = process.env.GH_TOKEN; + if (!token) { + throw new Error("GH_TOKEN not set in .env"); + } + + const octokit = new Octokit({ auth: token }); + const result = await fetchAndFormatPrDiff({ + octokit, + owner: "pullfrog", + repo: "scratch", + pullNumber: 49, + }); + + // verify TOC structure + expect(result.toc).toContain("## Files"); + expect(result.toc).toContain("→ lines"); + + // verify content includes TOC at the start + expect(result.content.startsWith(result.toc)).toBe(true); + + // verify content includes diff headers + expect(result.content).toContain("diff --git"); + expect(result.content).toContain("---"); + expect(result.content).toContain("+++"); + + // snapshot the full output + expect(result.toc).toMatchSnapshot("toc"); + expect(result.content).toMatchSnapshot("content"); + }); +}); diff --git a/mcp/checkout.ts b/mcp/checkout.ts index 11592f3..0277f9c 100644 --- a/mcp/checkout.ts +++ b/mcp/checkout.ts @@ -9,23 +9,43 @@ import { execute, tool } from "./shared.ts"; type PullFile = RestEndpointMethodTypes["pulls"]["listFiles"]["response"]["data"][number]; +export type FormatFilesResult = { + content: string; + toc: string; +}; + /** * formats PR files with explicit line numbers for each code line. * preserves all original diff info (file headers, hunk headers) and adds: * | OLD | NEW | TYPE | code + * returns both the formatted content and a TOC with line ranges per file. */ -export function formatFilesWithLineNumbers(files: PullFile[]): string { +export function formatFilesWithLineNumbers(files: PullFile[]): FormatFilesResult { const output: string[] = []; + const tocEntries: Array<{ filename: string; startLine: number; endLine: number }> = []; + + // calculate TOC header size: "## Files (N)\n" + N entries + "\n---\n\n" + const tocHeaderSize = 1 + files.length + 2; + let currentLine = tocHeaderSize + 1; for (const file of files) { + const fileStartLine = currentLine; + // file header output.push(`diff --git a/${file.filename} b/${file.filename}`); output.push(`--- a/${file.filename}`); output.push(`+++ b/${file.filename}`); + currentLine += 3; if (!file.patch) { output.push("(binary file or no changes)"); output.push(""); + currentLine += 2; + tocEntries.push({ + filename: file.filename, + startLine: fileStartLine, + endLine: currentLine - 1, + }); continue; } @@ -41,6 +61,7 @@ export function formatFilesWithLineNumbers(files: PullFile[]): string { oldLine = parseInt(hunkMatch[1], 10); newLine = parseInt(hunkMatch[2], 10); output.push(line); // pass through unchanged + currentLine++; continue; } @@ -69,11 +90,31 @@ export function formatFilesWithLineNumbers(files: PullFile[]): string { // unknown line type, pass through output.push(line); } + currentLine++; } output.push(""); // blank line between files + currentLine++; + + tocEntries.push({ + filename: file.filename, + startLine: fileStartLine, + endLine: currentLine - 1, + }); } - return output.join("\n"); + // build TOC + const tocLines = [`## Files (${files.length})`]; + for (const entry of tocEntries) { + tocLines.push(`- ${entry.filename} → lines ${entry.startLine}-${entry.endLine}`); + } + tocLines.push(""); + tocLines.push("---"); + tocLines.push(""); + + const toc = tocLines.join("\n"); + const content = toc + output.join("\n"); + + return { content, toc }; } function padNum(n: number): string { @@ -97,6 +138,27 @@ export type CheckoutPrResult = { diffPath: string; }; +type FetchPrDiffParams = { + octokit: Octokit; + owner: string; + repo: string; + pullNumber: number; +}; + +/** + * fetches PR files from GitHub and formats them with line numbers and TOC. + * this is the core diff formatting logic, extracted for testability. + */ +export async function fetchAndFormatPrDiff(params: FetchPrDiffParams): Promise { + const filesResponse = await params.octokit.rest.pulls.listFiles({ + owner: params.owner, + repo: params.repo, + pull_number: params.pullNumber, + per_page: 100, + }); + return formatFilesWithLineNumbers(filesResponse.data); +} + interface CheckoutPrBranchParams { octokit: Octokit; owner: string; @@ -243,14 +305,13 @@ export function CheckoutPrTool(ctx: ToolContext) { } // fetch PR files and format with line numbers - const filesResponse = await ctx.octokit.rest.pulls.listFiles({ + const formatResult = await fetchAndFormatPrDiff({ + octokit: ctx.octokit, owner: ctx.repo.owner, repo: ctx.repo.name, - pull_number, - per_page: 100, + pullNumber: pull_number, }); - const diffContent = formatFilesWithLineNumbers(filesResponse.data); - const diffPreview = diffContent.split("\n").slice(0, 100).join("\n"); + const diffPreview = formatResult.content.split("\n").slice(0, 100).join("\n"); log.debug(`formatted diff preview (first 100 lines):\n${diffPreview}`); const tempDir = process.env.PULLFROG_TEMP_DIR; if (!tempDir) { @@ -259,8 +320,8 @@ export function CheckoutPrTool(ctx: ToolContext) { ); } const diffPath = join(tempDir, `pr-${pull_number}.diff`); - writeFileSync(diffPath, diffContent); - log.debug(`wrote diff to ${diffPath} (${diffContent.length} bytes)`); + writeFileSync(diffPath, formatResult.content); + log.debug(`wrote diff to ${diffPath} (${formatResult.content.length} bytes)`); return { success: true, diff --git a/mcp/commitInfo.ts b/mcp/commitInfo.ts index fd02577..66fc35a 100644 --- a/mcp/commitInfo.ts +++ b/mcp/commitInfo.ts @@ -28,7 +28,7 @@ export function CommitInfoTool(ctx: ToolContext) { const files = data.files ?? []; // format diff with line numbers and write to file - const diffContent = formatFilesWithLineNumbers(files); + const formatResult = formatFilesWithLineNumbers(files); const tempDir = process.env.PULLFROG_TEMP_DIR; if (!tempDir) { throw new Error( @@ -36,8 +36,8 @@ export function CommitInfoTool(ctx: ToolContext) { ); } const diffFile = join(tempDir, `commit-${sha.slice(0, 7)}.diff`); - writeFileSync(diffFile, diffContent); - log.debug(`wrote commit diff to ${diffFile} (${diffContent.length} bytes)`); + writeFileSync(diffFile, formatResult.content); + log.debug(`wrote commit diff to ${diffFile} (${formatResult.content.length} bytes)`); return { sha: data.sha, diff --git a/mcp/reviewComments.test.ts b/mcp/reviewComments.test.ts new file mode 100644 index 0000000..8f83742 --- /dev/null +++ b/mcp/reviewComments.test.ts @@ -0,0 +1,57 @@ +import { Octokit } from "@octokit/rest"; +import { describe, expect, it } from "vitest"; +import { + buildThreadBlocks, + formatReviewThreads, + type ParsedHunk, + parseFilePatches, + REVIEW_THREADS_QUERY, + type ReviewThread, + type ReviewThreadsQueryResponse, +} from "./reviewComments.ts"; + +describe("formatReviewThreads", () => { + it("formats thread blocks with TOC and correct line numbers", async () => { + const token = process.env.GH_TOKEN; + if (!token) { + throw new Error("GH_TOKEN is not set"); + } + + const octokit = new Octokit({ auth: token }); + const pullNumber = 49; + const reviewId = 3485940013; + + // fetch review threads via GraphQL + const response = await octokit.graphql(REVIEW_THREADS_QUERY, { + owner: "pullfrog", + name: "scratch", + prNumber: pullNumber, + }); + + const allThreads = response.repository?.pullRequest?.reviewThreads?.nodes ?? []; + const threadsForReview = allThreads.filter((thread): thread is ReviewThread => { + if (!thread?.comments?.nodes) return false; + return thread.comments.nodes.some((c) => c?.pullRequestReview?.databaseId === reviewId); + }); + + // fetch file patches + const prFilesResponse = await octokit.rest.pulls.listFiles({ + owner: "pullfrog", + repo: "scratch", + pull_number: pullNumber, + }); + const filePatchMap = new Map(); + for (const file of prFilesResponse.data) { + if (file.patch) { + filePatchMap.set(file.filename, parseFilePatches(file.patch)); + } + } + + // build and format + const { threadBlocks, reviewer } = buildThreadBlocks(threadsForReview, filePatchMap, reviewId); + const result = formatReviewThreads(threadBlocks, { pullNumber, reviewId, reviewer }); + + expect(result.toc).toMatchSnapshot("toc"); + expect(result.content).toMatchSnapshot("content"); + }); +}); diff --git a/mcp/reviewComments.ts b/mcp/reviewComments.ts index 1fe2546..13288cb 100644 --- a/mcp/reviewComments.ts +++ b/mcp/reviewComments.ts @@ -5,64 +5,42 @@ import { log } from "../utils/log.ts"; import type { ToolContext } from "./server.ts"; import { execute, tool } from "./shared.ts"; -// fragment for nested replyTo (5 levels deep covers most threads) -// because in_reply_to_id generally points to the top-level comment -// this doesn't actually work as expected -// TOD: implement an API endpoint with aggressive caching that fetches the PR's reviewThreads via graphql -// separately fetch all the comments associated with the particular review -// merge them, then construct the full thread context -const REPLY_TO_FRAGMENT = ` - replyTo { - databaseId - body - author { login } - replyTo { - databaseId - body - author { login } - replyTo { - databaseId - body - author { login } - replyTo { - databaseId - body - author { login } - replyTo { - databaseId - body - author { login } - } - } - } - } - } -`; - -// fetch specific review by node ID with nested thread context (single efficient query) -const REVIEW_QUERY = ` -query ($nodeId: ID!) { - node(id: $nodeId) { - ... on PullRequestReview { - databaseId - author { login } - comments(first: 100) { +// GraphQL query to fetch all review threads for a PR with full comment history +export const REVIEW_THREADS_QUERY = ` +query ($owner: String!, $name: String!, $prNumber: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $prNumber) { + reviewThreads(first: 100) { nodes { - databaseId - body + id path line startLine - diffHunk - url - author { login } - createdAt - ${REPLY_TO_FRAGMENT} - reactionGroups { - content - reactors(first: 10) { - nodes { - ... on Actor { login } + diffSide + isResolved + isOutdated + comments(first: 50) { + nodes { + fullDatabaseId + body + createdAt + diffHunk + line + startLine + originalLine + originalStartLine + author { login } + pullRequestReview { + databaseId + author { login } + } + reactionGroups { + content + reactors(first: 10) { + nodes { + ... on Actor { login } + } + } } } } @@ -73,208 +51,494 @@ query ($nodeId: ID!) { } `; -// nested replyTo type (recursive up to 5 levels) -type NestedReplyTo = { - databaseId: number; +export type ReviewThreadComment = { + fullDatabaseId: string | null; body: string; + createdAt: string; + diffHunk: string; + line: number | null; + startLine: number | null; + originalLine: number | null; + originalStartLine: number | null; author: { login: string } | null; - replyTo?: NestedReplyTo | null; -} | null; + pullRequestReview: { + databaseId: number | null; + author: { login: string } | null; + } | null; + reactionGroups: Array<{ + content: string; + reactors: { nodes: Array<{ login: string } | null> | null } | null; + }> | null; +}; -type ReviewComment = { - databaseId: number; - body: string; +export type ReviewThread = { + id: string; path: string; line: number | null; startLine: number | null; - diffHunk: string; - url: string; - author: { login: string } | null; - createdAt: string; - replyTo: NestedReplyTo; - reactionGroups: - | { - content: string; - reactors: { nodes: ({ login: string } | null)[] | null }; - }[] - | null; + diffSide: "LEFT" | "RIGHT"; + isResolved: boolean; + isOutdated: boolean; + comments: { + nodes: (ReviewThreadComment | null)[] | null; + } | null; }; -type ReviewQueryResponse = { - node: { - databaseId: number; - author: { login: string } | null; - comments: { - nodes: (ReviewComment | null)[] | null; +export type ReviewThreadsQueryResponse = { + repository: { + pullRequest: { + reviewThreads: { + nodes: (ReviewThread | null)[] | null; + } | null; } | null; } | null; }; -const MAX_BODY_PREVIEW = 80; +// extract exactly the commented line range from diffHunk, plus context +const CONTEXT_PADDING = 3; -function truncateBody(body: string): string { - const oneLine = body.replace(/\n/g, " ").trim(); - if (oneLine.length <= MAX_BODY_PREVIEW) return oneLine; - return oneLine.slice(0, MAX_BODY_PREVIEW - 3) + "..."; +function extractCommentedLines( + diffHunk: string, + startLine: number | null, + endLine: number | null, + side: "LEFT" | "RIGHT" +): string { + const lines = diffHunk.split("\n"); + if (lines.length <= 1) return diffHunk; + + const header = lines[0]; + const contentLines = lines.slice(1); + + // parse header: @@ -old_start,old_count +new_start,new_count @@ + const headerMatch = header.match(/@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/); + if (!headerMatch) return diffHunk; + + const hunkOldStart = parseInt(headerMatch[1], 10); + const hunkNewStart = parseInt(headerMatch[2], 10); + + // LEFT = old file (deletions), RIGHT = new file (additions) + const hunkStart = side === "LEFT" ? hunkOldStart : hunkNewStart; + const commentStart = startLine ?? endLine ?? hunkStart; + const commentEnd = endLine ?? commentStart; + + // walk through diff lines, tracking line numbers for both old and new files + // - lines: old file only (LEFT) + // + lines: new file only (RIGHT) + // context lines: both files + type DiffLine = { text: string; lineNum: number | null }; + const diffLines: DiffLine[] = []; + let oldLineNum = hunkOldStart; + let newLineNum = hunkNewStart; + + for (const line of contentLines) { + const prefix = line[0]; + if (prefix === "-") { + // deletion - only has old line number + diffLines.push({ text: line, lineNum: side === "LEFT" ? oldLineNum : null }); + oldLineNum++; + } else if (prefix === "+") { + // addition - only has new line number + diffLines.push({ text: line, lineNum: side === "RIGHT" ? newLineNum : null }); + newLineNum++; + } else { + // context - has both line numbers + diffLines.push({ text: line, lineNum: side === "LEFT" ? oldLineNum : newLineNum }); + oldLineNum++; + newLineNum++; + } + } + + // find lines for comment range with context + const targetStart = commentStart - CONTEXT_PADDING; + const targetEnd = commentEnd; + + const result: string[] = []; + let truncatedBefore = 0; + + for (let i = 0; i < diffLines.length; i++) { + const dl = diffLines[i]; + // include if: within target range, OR it's an "other side" line adjacent to included lines + const inRange = dl.lineNum !== null && dl.lineNum >= targetStart && dl.lineNum <= targetEnd; + // include opposite-side lines if they're between included lines + const adjacentOtherSide = dl.lineNum === null && result.length > 0 && i < diffLines.length - 1; + + if (inRange || adjacentOtherSide) { + result.push(dl.text); + } else if (result.length === 0) { + truncatedBefore++; + } + } + + if (truncatedBefore > 0) { + return `${header}\n... (${truncatedBefore} lines above) ...\n${result.join("\n")}`; + } + return `${header}\n${result.join("\n")}`; } -function escapeXml(text: string): string { - return text - .replace(/&/g, "&") - .replace(//g, ">") - .replace(/"/g, """); +// parsed hunk from a unified diff +export type ParsedHunk = { + header: string; + oldStart: number; + oldCount: number; + newStart: number; + newCount: number; + content: string[]; +}; + +// parse a full file patch into individual hunks +export function parseFilePatches(patch: string): ParsedHunk[] { + const hunks: ParsedHunk[] = []; + const lines = patch.split("\n"); + + let currentHunk: ParsedHunk | null = null; + + for (const line of lines) { + const hunkMatch = line.match(/@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/); + if (hunkMatch) { + if (currentHunk) hunks.push(currentHunk); + currentHunk = { + header: line, + oldStart: parseInt(hunkMatch[1], 10), + oldCount: parseInt(hunkMatch[2] ?? "1", 10), + newStart: parseInt(hunkMatch[3], 10), + newCount: parseInt(hunkMatch[4] ?? "1", 10), + content: [], + }; + } else if (currentHunk) { + currentHunk.content.push(line); + } + } + if (currentHunk) hunks.push(currentHunk); + + return hunks; } -function hasThumbsUpFrom(comment: ReviewComment, username: string): boolean { - if (!comment.reactionGroups) return false; - const thumbsUp = comment.reactionGroups.find((g) => g.content === "THUMBS_UP"); - if (!thumbsUp?.reactors?.nodes) return false; - return thumbsUp.reactors.nodes.some((r) => r?.login === username); +// find hunks that overlap with a line range (for LEFT or RIGHT side) +function findOverlappingHunks( + hunks: ParsedHunk[], + startLine: number, + endLine: number, + side: "LEFT" | "RIGHT" +): ParsedHunk[] { + return hunks.filter((hunk) => { + const hunkStart = side === "LEFT" ? hunk.oldStart : hunk.newStart; + const hunkCount = side === "LEFT" ? hunk.oldCount : hunk.newCount; + const hunkEnd = hunkStart + hunkCount - 1; + + // check for overlap: ranges overlap if start1 <= end2 && start2 <= end1 + return startLine <= hunkEnd && hunkStart <= endLine; + }); } -// flatten nested replyTo chain into array (oldest first) -function flattenReplyToChain(replyTo: NestedReplyTo): Array<{ body: string; author: string }> { - if (!replyTo) return []; - const parent = flattenReplyToChain(replyTo.replyTo ?? null); - return [...parent, { body: replyTo.body, author: replyTo.author?.login ?? "unknown" }]; +// extract diff content from multiple hunks for a comment range +function extractFromFilePatches( + hunks: ParsedHunk[], + startLine: number, + endLine: number, + side: "LEFT" | "RIGHT" +): string { + const overlapping = findOverlappingHunks(hunks, startLine, endLine, side); + + if (overlapping.length === 0) { + return `(no diff hunks found for lines ${startLine}-${endLine})`; + } + + if (overlapping.length === 1) { + // single hunk - use existing extraction logic + const hunk = overlapping[0]; + const fullHunk = hunk.header + "\n" + hunk.content.join("\n"); + return extractCommentedLines(fullHunk, startLine, endLine, side); + } + + // multiple hunks - combine them with gap indicators + const result: string[] = []; + let prevHunkEnd = 0; + + for (let i = 0; i < overlapping.length; i++) { + const hunk = overlapping[i]; + const hunkStart = side === "LEFT" ? hunk.oldStart : hunk.newStart; + const hunkCount = side === "LEFT" ? hunk.oldCount : hunk.newCount; + const hunkEnd = hunkStart + hunkCount - 1; + + // add gap indicator if there's a gap between hunks + if (i > 0 && hunkStart > prevHunkEnd + 1) { + const gapSize = hunkStart - prevHunkEnd - 1; + result.push(`\n... (${gapSize} unchanged lines) ...\n`); + } + + // add the hunk header and content + result.push(hunk.header); + result.push(...hunk.content); + + prevHunkEnd = hunkEnd; + } + + return result.join("\n"); } export const GetReviewComments = type({ pull_number: type.number.describe("The pull request number"), review_id: type.number.describe("The review ID to get comments for"), approved_by: type.string - .describe("Optional GitHub username - only return comments this user gave a 👍 to") + .describe( + "Optional GitHub username - only return threads where this user gave a 👍 to at least one comment" + ) .optional(), }); +function hasThumbsUpFrom(comment: ReviewThreadComment, username: string): boolean { + if (!comment.reactionGroups) return false; + const thumbsUp = comment.reactionGroups.find((g) => g.content === "THUMBS_UP"); + if (!thumbsUp?.reactors?.nodes) return false; + return thumbsUp.reactors.nodes.some((r) => r?.login === username); +} + +function threadHasThumbsUpFrom(thread: ReviewThread, username: string): boolean { + const comments = thread.comments?.nodes ?? []; + return comments.some((c) => c && hasThumbsUpFrom(c, username)); +} + +/** + * formats thread blocks into markdown with TOC and line numbers. + * extracted for testability. + */ +export function formatReviewThreads( + threadBlocks: Array<{ path: string; lineRange: string; content: string[] }>, + header: { pullNumber: number; reviewId: number; reviewer: string } +) { + // header section takes: title (1) + blank (1) + "## TOC" (1) + blank (1) + N TOC entries + blank (1) + "---" (1) + blank (1) + const tocHeaderLines = 4; + const tocFooterLines = 3; + let currentLine = tocHeaderLines + threadBlocks.length + tocFooterLines + 1; + + const tocEntries: string[] = []; + const threadLines: string[] = []; + + for (const block of threadBlocks) { + const startLine = currentLine; + const actualLineCount = block.content.reduce((sum, line) => sum + line.split("\n").length, 0); + const endLine = currentLine + actualLineCount - 1; + tocEntries.push(`- ${block.path}:${block.lineRange} → lines ${startLine}-${endLine}`); + threadLines.push(...block.content); + currentLine += actualLineCount; + } + + const lines: string[] = []; + lines.push( + `# Review Threads (${threadBlocks.length}) for PR #${header.pullNumber} - Review ${header.reviewId} by ${header.reviewer}` + ); + lines.push(""); + lines.push("## TOC"); + lines.push(""); + lines.push(...tocEntries); + lines.push(""); + lines.push("---"); + lines.push(""); + lines.push(...threadLines); + + return { + toc: tocEntries.join("\n"), + content: lines.join("\n"), + }; +} + +/** + * builds thread blocks from review threads and file patches. + * extracted for testability. + */ +export function buildThreadBlocks( + threads: ReviewThread[], + filePatchMap: Map, + reviewId: number +) { + // get reviewer from first matching comment + const firstMatchingComment = threads[0]?.comments?.nodes?.find( + (c) => c?.pullRequestReview?.databaseId === reviewId + ); + const reviewer = firstMatchingComment?.pullRequestReview?.author?.login ?? "unknown"; + + // sort threads by file path, then by line number + threads.sort((a, b) => { + const pathCmp = a.path.localeCompare(b.path); + if (pathCmp !== 0) return pathCmp; + const aLine = a.startLine ?? a.line ?? 0; + const bLine = b.startLine ?? b.line ?? 0; + return aLine - bLine; + }); + + const threadBlocks: Array<{ path: string; lineRange: string; content: string[] }> = []; + + for (const thread of threads) { + const allComments = (thread.comments?.nodes ?? []).filter( + (c): c is ReviewThreadComment => c !== null + ); + if (allComments.length === 0) continue; + + // get line info from thread, or fall back to first comment's line info + const firstComment = allComments[0]; + const line = + thread.line ?? firstComment?.line ?? firstComment?.originalLine ?? thread.startLine ?? 0; + const startLine = + thread.startLine ?? firstComment?.startLine ?? firstComment?.originalStartLine ?? line; + const lineRange = startLine === line ? `${line}` : `${startLine}-${line}`; + const block: string[] = []; + + // header with file:line range and status + const status = thread.isResolved ? " [RESOLVED]" : thread.isOutdated ? " [OUTDATED]" : ""; + block.push(`## ${thread.path}:${lineRange}${status}`); + block.push(""); + + // show all comments in the thread (full conversation history) + for (const comment of allComments) { + const author = comment.author?.login ?? "unknown"; + const isTargetReview = comment.pullRequestReview?.databaseId === reviewId; + const marker = isTargetReview ? " *" : ""; + + block.push( + `\`\`\`\`comment author=${author} id=${comment.fullDatabaseId ?? "unknown"} review=${comment.pullRequestReview?.databaseId ?? "unknown"}${marker}` + ); + block.push(comment.body || "(no comment body)"); + block.push("````"); + block.push(""); + } + + // diff context + const fileHunks = filePatchMap.get(thread.path); + const firstCommentWithHunk = allComments.find((c) => c.diffHunk); + let diffContent: string | null = null; + + if (fileHunks && fileHunks.length > 0) { + const overlapping = findOverlappingHunks(fileHunks, startLine, line, thread.diffSide); + if (overlapping.length > 0) { + diffContent = extractFromFilePatches(fileHunks, startLine, line, thread.diffSide); + } + } + + if (!diffContent && firstCommentWithHunk) { + diffContent = extractCommentedLines( + firstCommentWithHunk.diffHunk, + startLine, + line, + thread.diffSide + ); + } + + if (diffContent) { + block.push(`\`\`\`diff file=${thread.path} lines=${lineRange} side=${thread.diffSide}`); + block.push(diffContent); + block.push("```"); + block.push(""); + } else { + block.push(`\`\`\`diff file=${thread.path} lines=${lineRange} side=${thread.diffSide}`); + block.push(`(no diff context available - comment on unchanged lines)`); + block.push("```"); + block.push(""); + } + + threadBlocks.push({ path: thread.path, lineRange, content: block }); + } + + return { threadBlocks, reviewer }; +} + export function GetReviewCommentsTool(ctx: ToolContext) { return tool({ name: "get_review_comments", description: - "Get review comments for a pull request review, including thread context. " + - "When approved_by is provided, only returns comments that user approved with 👍. " + - "Returns commentsPath pointing to a file with full comment details in XML format.", + "Get review comments for a pull request review with full thread context. " + + "When approved_by is provided, only returns threads where that user gave a 👍 to at least one comment. " + + "Returns a TOC and commentsPath pointing to a markdown file with full comment details.", parameters: GetReviewComments, - execute: execute(async ({ pull_number, review_id, approved_by }) => { - // fetch the review to get node_id and reviewer - const { data: review } = await ctx.octokit.rest.pulls.getReview({ + execute: execute(async (params) => { + // fetch all review threads for the PR via GraphQL + const response = await ctx.octokit.graphql(REVIEW_THREADS_QUERY, { owner: ctx.repo.owner, - repo: ctx.repo.name, - pull_number, - review_id, + name: ctx.repo.name, + prNumber: params.pull_number, }); - const reviewer = review.user?.login ?? "unknown"; + const allThreads = response.repository?.pullRequest?.reviewThreads?.nodes ?? []; - // fetch comments with nested thread context via GraphQL - const response = await ctx.octokit.graphql(REVIEW_QUERY, { - nodeId: review.node_id, + // filter to threads where at least one comment belongs to the target review + let threadsForReview = allThreads.filter((thread): thread is ReviewThread => { + if (!thread?.comments?.nodes) return false; + return thread.comments.nodes.some( + (c) => c?.pullRequestReview?.databaseId === params.review_id + ); }); - const reviewComments = response.node?.comments?.nodes; - if (!reviewComments) { - return { - review_id, - pull_number, - reviewer, - count: 0, - commentsPath: null, - message: "No comments found for this review", - }; - } - - const allComments = reviewComments.filter((c): c is ReviewComment => c !== null); - // filter by approved_by if specified - const comments = approved_by - ? allComments.filter((c) => hasThumbsUpFrom(c, approved_by)) - : allComments; + if (params.approved_by) { + threadsForReview = threadsForReview.filter((thread) => + threadHasThumbsUpFrom(thread, params.approved_by as string) + ); + } - if (comments.length === 0) { + if (threadsForReview.length === 0) { return { - review_id, - pull_number, - reviewer, - count: 0, + review_id: params.review_id, + pull_number: params.pull_number, + reviewer: "unknown", + threadCount: 0, commentsPath: null, - message: approved_by - ? `No comments with 👍 from ${approved_by}` - : "No comments found for this review", + toc: null, + instructions: params.approved_by + ? `no threads with 👍 from ${params.approved_by}` + : "no threads found for this review", }; } - // build XML output - const lines: string[] = []; - lines.push(``); - lines.push(""); - - // summary section - lines.push(""); - for (const comment of comments) { - const line = comment.line ?? comment.startLine ?? 0; - const preview = escapeXml(truncateBody(comment.body)); - lines.push( - ` ${preview}` - ); - } - lines.push(""); - lines.push(""); - - // detailed comments with thread context - for (const comment of comments) { - const line = comment.line ?? comment.startLine ?? 0; - const author = comment.author?.login ?? "unknown"; - lines.push( - `` - ); - - // thread history (parent comments from nested replyTo) - const thread = flattenReplyToChain(comment.replyTo); - if (thread.length > 0) { - lines.push(" "); - for (const msg of thread) { - lines.push( - ` ${escapeXml(msg.body)}` - ); - } - lines.push(" "); + // fetch full file patches for better multi-hunk context + const prFilesResponse = await ctx.octokit.rest.pulls.listFiles({ + owner: ctx.repo.owner, + repo: ctx.repo.name, + pull_number: params.pull_number, + }); + const filePatchMap = new Map(); + for (const file of prFilesResponse.data) { + if (file.patch) { + filePatchMap.set(file.filename, parseFilePatches(file.patch)); } - - // diff context - lines.push(" "); - lines.push(escapeXml(comment.diffHunk)); - lines.push(" "); - - // the actual comment body to address - lines.push(` ${escapeXml(comment.body)}`); - lines.push(""); - lines.push(""); } - lines.push(""); + // build thread blocks + const { threadBlocks, reviewer } = buildThreadBlocks( + threadsForReview, + filePatchMap, + params.review_id + ); - const content = lines.join("\n"); + // format thread blocks into markdown with TOC + const formatted = formatReviewThreads(threadBlocks, { + pullNumber: params.pull_number, + reviewId: params.review_id, + reviewer, + }); // write to temp file const tempDir = process.env.PULLFROG_TEMP_DIR; if (!tempDir) { throw new Error("PULLFROG_TEMP_DIR not set"); } - const filename = approved_by - ? `review-${review_id}-approved-by-${approved_by}.xml` - : `review-${review_id}-comments.xml`; + const filename = `review-${params.review_id}-threads.md`; const commentsPath = join(tempDir, filename); - writeFileSync(commentsPath, content); - log.info(`wrote ${comments.length} comments to ${commentsPath}`); - log.box(content); + writeFileSync(commentsPath, formatted.content); + log.debug(`wrote ${threadBlocks.length} threads to ${commentsPath}`); return { - review_id, - pull_number, + review_id: params.review_id, + pull_number: params.pull_number, reviewer, - count: comments.length, + threadCount: threadBlocks.length, commentsPath, + toc: formatted.toc, + instructions: + `the file at commentsPath contains ${threadBlocks.length} review threads with full conversation history. ` + + `comments marked with * are from the target review (${params.review_id}). ` + + `the TOC shows each thread's file:line and the line number where it appears in the file. ` + + `to read a specific thread, use: grep -A 50 "^## " ${commentsPath} ` + + `(replace with the path from the TOC, e.g. "^## action/utils/foo.ts:42"). ` + + `address each thread in order, working through one file at a time.`, }; }), }); @@ -290,15 +554,15 @@ export function ListPullRequestReviewsTool(ctx: ToolContext) { description: "List all reviews for a pull request. Returns all reviews including approvals, request changes, and comments.", parameters: ListPullRequestReviews, - execute: execute(async ({ pull_number }) => { + execute: execute(async (params) => { const reviews = await ctx.octokit.paginate(ctx.octokit.rest.pulls.listReviews, { owner: ctx.repo.owner, repo: ctx.repo.name, - pull_number, + pull_number: params.pull_number, }); return { - pull_number, + pull_number: params.pull_number, reviews: reviews.map((review) => ({ id: review.id, node_id: review.node_id, diff --git a/mcp/shared.ts b/mcp/shared.ts index 6f4186f..9b9f652 100644 --- a/mcp/shared.ts +++ b/mcp/shared.ts @@ -4,7 +4,9 @@ import type { FastMCP, Tool } from "fastmcp"; import { formatJsonValue, log } from "../utils/cli.ts"; import type { ToolContext } from "./server.ts"; -export const tool = (toolDef: Tool>) => toolDef; +export const tool = ( + toolDef: Tool> +): Tool> => toolDef; export interface ToolResult { content: { @@ -40,11 +42,11 @@ export const handleToolError = (error: unknown): ToolResult => { * @param fn - the function to execute * @param toolName - optional tool name for error logging */ -export const execute = ( - fn: (params: T) => Promise | string>, +export const execute = | string>( + fn: (params: T) => Promise, toolName?: string ) => { - return async (params: T): Promise => { + const _fn = async (params: T): Promise => { try { const result = await fn(params); return handleToolSuccess(result); @@ -56,6 +58,8 @@ export const execute = ( return handleToolError(error); } }; + (_fn as any).raw = fn; + return _fn; }; /** diff --git a/vitest.config.ts b/vitest.config.ts index 1d0ca28..d3257cc 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,9 +1,10 @@ -import { defineConfig } from 'vitest/config'; +import { defineConfig } from "vitest/config"; export default defineConfig({ test: { globals: true, - environment: 'node', - exclude: ['node_modules', '.temp'], + environment: "node", + exclude: ["**/node_modules/**", "**/.temp/**", "**/.pnpm-store/**"], + setupFiles: ["./vitest.setup.ts"], }, }); diff --git a/vitest.setup.ts b/vitest.setup.ts new file mode 100644 index 0000000..771ac42 --- /dev/null +++ b/vitest.setup.ts @@ -0,0 +1,9 @@ +import { resolve } from "node:path"; +import { config } from "dotenv"; + +config({ path: resolve(import.meta.dirname, "../.env") }); + +// alias GITHUB_TOKEN to GH_TOKEN for tests +if (!process.env.GH_TOKEN && process.env.GITHUB_TOKEN) { + process.env.GH_TOKEN = process.env.GITHUB_TOKEN; +}