tests and better diffs (#163)

* refactor get_review_comments to use reviewThreads graphql api with full thread context and proper diff extraction

* Improve get_review_comments output

* Improve tests and diffs

* GH_TOKEN

* Added back approved_by

* Fix CI
This commit is contained in:
Colin McDonnell
2026-01-23 06:28:22 +00:00
committed by pullfrog[bot]
parent 9a8db3e07c
commit 7621d6f0e5
11 changed files with 1196 additions and 370 deletions
+359 -153
View File
@@ -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, "&amp;").replace(/</g, "&lt;").replace(/>/g, "&gt;").replace(/"/g, "&quot;");
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(`<review_comments count="${comments.length}" reviewer="${escapeXml(reviewer)}">`);
lines.push("");
lines.push("<summary>");
for (const comment of comments) {
const line = comment.line ?? comment.startLine ?? 0;
const preview = escapeXml(truncateBody(comment.body));
lines.push(
` <comment id="${comment.databaseId}" file="${escapeXml(comment.path)}" line="${line}">${preview}</comment>`
);
}
lines.push("</summary>");
lines.push("");
for (const comment of comments) {
const line = comment.line ?? comment.startLine ?? 0;
const author = comment.author?.login ?? "unknown";
lines.push(
`<comment id="${comment.databaseId}" file="${escapeXml(comment.path)}" line="${line}" author="${escapeXml(author)}">`
);
const thread = flattenReplyToChain(comment.replyTo);
if (thread.length > 0) {
lines.push(" <thread>");
for (const msg of thread) {
lines.push(
` <message author="${escapeXml(msg.author)}">${escapeXml(msg.body)}</message>`
);
}
lines.push(" </thread>");
const filePatchMap = /* @__PURE__ */ new Map();
for (const file2 of prFilesResponse.data) {
if (file2.patch) {
filePatchMap.set(file2.filename, parseFilePatches(file2.patch));
}
lines.push(" <diff>");
lines.push(escapeXml(comment.diffHunk));
lines.push(" </diff>");
lines.push(` <body>${escapeXml(comment.body)}</body>`);
lines.push("</comment>");
lines.push("");
}
lines.push("</review_comments>");
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 "^## <file:line>" ${commentsPath} (replace <file:line> 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,