Add "Fix it" link for body-only PR reviews (#338)

* Add "Fix it" link for body-only PR reviews

When a PR review has only body-level feedback (no inline comments),
the footer now includes a "Fix it" link that triggers the fix flow.

Also fetches the review body in the fix action's prompt so the agent
can address body-level feedback even when there are no inline comments.

* Move review body fetching into `get_review_comments` tool

Instead of fetching the review body in the trigger page and appending
it to the prompt, the `get_review_comments` MCP tool now fetches the
review body via the GitHub API and includes it in its markdown output
under a "Review Body" section. This keeps the trigger page simple and
lets the tool provide all review context in one place.

* fetch body early

* get reviewer from a better place

* cleanup structure to reuse more in test

* simplify

* simplify

* typecheck

* fetch review body via REST API; skip listFiles for body-only reviews

* update snapshot

* formatting

* cleanup

* fix line counting with `countNewlines` utility using `indexOf` loop

* rename `countNewlines` to `countLines` with 1-based line counting

* suppress biome lint warning for assignment in while condition

* remove unused `body` field from GraphQL review query and type

* add `approved` parameter to `create_pull_request_review` and skip fix links for approvals

* vibe instructions

* tighten up prompting

---------

Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
This commit is contained in:
pullfrog[bot]
2026-02-27 12:18:51 +00:00
committed by pullfrog[bot]
parent e0bd984975
commit c0fd69560f
6 changed files with 310 additions and 182 deletions
+109 -62
View File
@@ -143257,6 +143257,9 @@ var CreatePullRequestReview = type({
body: type.string.describe( body: type.string.describe(
"1-2 sentence high-level summary with urgency level, critical callouts, and feedback about code outside the diff. Specific feedback on diff lines goes in 'comments' array." "1-2 sentence high-level summary with urgency level, critical callouts, and feedback about code outside the diff. Specific feedback on diff lines goes in 'comments' array."
).optional(), ).optional(),
approved: type.boolean.describe(
"Set to true to submit as an approval. ONLY when the review contains no actionable feedback \u2014 neither inline comments nor actionable content in the body. Defaults to false (comment-only review). Rejections are not supported."
).optional(),
commit_id: type.string.describe("Optional SHA of the commit being reviewed. Defaults to latest.").optional(), commit_id: type.string.describe("Optional SHA of the commit being reviewed. Defaults to latest.").optional(),
comments: type({ comments: type({
path: type.string.describe( path: type.string.describe(
@@ -143284,13 +143287,13 @@ function CreatePullRequestReviewTool(ctx) {
name: "create_pull_request_review", name: "create_pull_request_review",
description: `Submit a review for an existing pull request. IMPORTANT: 95%+ of feedback should be in 'comments' array with file paths and line numbers. Only use 'body' for a 1-2 sentence summary with urgency and critical callouts. Use 'suggestion' to propose replacement code - MUST preserve exact indentation of original code. Example replacing lines 42-44 (3 lines) with 5 lines: { path: 'src/api.ts', start_line: 42, line: 44, suggestion: ' const result = await fetch(url);\\n if (!result.ok) {\\n log.error(result.status);\\n throw new Error("request failed");\\n }' } CONSTRAINT: Inline comments can ONLY target files and lines that appear in the PR diff. Commenting on files or lines outside the diff will cause GitHub API errors. Put feedback about code outside the diff in 'body' instead.`, description: `Submit a review for an existing pull request. IMPORTANT: 95%+ of feedback should be in 'comments' array with file paths and line numbers. Only use 'body' for a 1-2 sentence summary with urgency and critical callouts. Use 'suggestion' to propose replacement code - MUST preserve exact indentation of original code. Example replacing lines 42-44 (3 lines) with 5 lines: { path: 'src/api.ts', start_line: 42, line: 44, suggestion: ' const result = await fetch(url);\\n if (!result.ok) {\\n log.error(result.status);\\n throw new Error("request failed");\\n }' } CONSTRAINT: Inline comments can ONLY target files and lines that appear in the PR diff. Commenting on files or lines outside the diff will cause GitHub API errors. Put feedback about code outside the diff in 'body' instead.`,
parameters: CreatePullRequestReview, parameters: CreatePullRequestReview,
execute: execute(async ({ pull_number, body, commit_id, comments = [] }) => { execute: execute(async ({ pull_number, body, approved, commit_id, comments = [] }) => {
ctx.toolState.issueNumber = pull_number; ctx.toolState.issueNumber = pull_number;
const params = { const params = {
owner: ctx.repo.owner, owner: ctx.repo.owner,
repo: ctx.repo.name, repo: ctx.repo.name,
pull_number, pull_number,
event: "COMMENT" event: approved ? "APPROVE" : "COMMENT"
}; };
if (body) params.body = body; if (body) params.body = body;
if (commit_id) { if (commit_id) {
@@ -143329,11 +143332,17 @@ function CreatePullRequestReviewTool(ctx) {
} }
const reviewId = result.data.id; const reviewId = result.data.id;
const customParts = []; const customParts = [];
if (comments.length > 0) { if (!approved) {
const apiUrl = getApiUrl(); if (comments.length > 0) {
const fixAllUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix&review_id=${reviewId}`; const apiUrl = getApiUrl();
const fixApprovedUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix-approved&review_id=${reviewId}`; const fixAllUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix&review_id=${reviewId}`;
customParts.push(`[Fix all \u2794](${fixAllUrl})`, `[Fix \u{1F44D}s \u2794](${fixApprovedUrl})`); const fixApprovedUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix-approved&review_id=${reviewId}`;
customParts.push(`[Fix all \u2794](${fixAllUrl})`, `[Fix \u{1F44D}s \u2794](${fixApprovedUrl})`);
} else if (body) {
const apiUrl = getApiUrl();
const fixUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix&review_id=${reviewId}`;
customParts.push(`[Fix it \u2794](${fixUrl})`);
}
} }
const footer = buildPullfrogFooter({ const footer = buildPullfrogFooter({
workflowRun: { workflowRun: {
@@ -143412,6 +143421,14 @@ query ($owner: String!, $name: String!, $prNumber: Int!) {
} }
} }
`; `;
function countLines(str) {
let count = 1;
let index = -1;
while ((index = str.indexOf("\n", index + 1)) !== -1) {
count++;
}
return count;
}
var CONTEXT_PADDING = 3; var CONTEXT_PADDING = 3;
function extractCommentedLines(diffHunk, startLine, endLine, side) { function extractCommentedLines(diffHunk, startLine, endLine, side) {
const lines = diffHunk.split("\n"); const lines = diffHunk.split("\n");
@@ -143535,8 +143552,8 @@ function hasThumbsUpFrom(comment, username) {
if (!comment.reactionGroups) return false; if (!comment.reactionGroups) return false;
const thumbsUp = comment.reactionGroups.find((g) => g.content === "THUMBS_UP"); const thumbsUp = comment.reactionGroups.find((g) => g.content === "THUMBS_UP");
if (!thumbsUp?.reactors?.nodes) return false; if (!thumbsUp?.reactors?.nodes) return false;
const usernameNeedle = username.toLowerCase(); const needle = username.toLowerCase();
return thumbsUp.reactors.nodes.some((r) => r?.login?.toLowerCase() === usernameNeedle); return thumbsUp.reactors.nodes.some((r) => r?.login?.toLowerCase() === needle);
} }
function threadHasThumbsUpFrom(thread, username) { function threadHasThumbsUpFrom(thread, username) {
const comments = thread.comments?.nodes ?? []; const comments = thread.comments?.nodes ?? [];
@@ -143546,11 +143563,16 @@ function formatReviewThreads(threadBlocks, header) {
const tocHeaderLines = 4; const tocHeaderLines = 4;
const tocFooterLines = 3; const tocFooterLines = 3;
let currentLine = tocHeaderLines + threadBlocks.length + tocFooterLines + 1; let currentLine = tocHeaderLines + threadBlocks.length + tocFooterLines + 1;
const reviewBodyLines = [];
if (header.reviewBody) {
reviewBodyLines.push("## Review Body", "", header.reviewBody, "");
currentLine += reviewBodyLines.reduce((sum, line) => sum + countLines(line), 0);
}
const tocEntries = []; const tocEntries = [];
const threadLines = []; const threadLines = [];
for (const block of threadBlocks) { for (const block of threadBlocks) {
const startLine = currentLine; const startLine = currentLine;
const actualLineCount = block.content.reduce((sum, line) => sum + line.split("\n").length, 0); const actualLineCount = block.content.reduce((sum, line) => sum + countLines(line), 0);
const endLine = currentLine + actualLineCount - 1; const endLine = currentLine + actualLineCount - 1;
tocEntries.push(`- ${block.path}:${block.lineRange} \u2192 lines ${startLine}-${endLine}`); tocEntries.push(`- ${block.path}:${block.lineRange} \u2192 lines ${startLine}-${endLine}`);
threadLines.push(...block.content); threadLines.push(...block.content);
@@ -143561,10 +143583,13 @@ function formatReviewThreads(threadBlocks, header) {
`# Review Threads (${threadBlocks.length}) for PR #${header.pullNumber} - Review ${header.reviewId} by ${header.reviewer}` `# Review Threads (${threadBlocks.length}) for PR #${header.pullNumber} - Review ${header.reviewId} by ${header.reviewer}`
); );
lines.push(""); lines.push("");
lines.push("## TOC"); if (threadBlocks.length > 0) {
lines.push(""); lines.push("## TOC");
lines.push(...tocEntries); lines.push("");
lines.push(""); lines.push(...tocEntries);
lines.push("");
}
lines.push(...reviewBodyLines);
lines.push("---"); lines.push("---");
lines.push(""); lines.push("");
lines.push(...threadLines); lines.push(...threadLines);
@@ -143574,10 +143599,6 @@ function formatReviewThreads(threadBlocks, header) {
}; };
} }
function buildThreadBlocks(threads, filePatchMap, reviewId) { 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) => { threads.sort((a, b) => {
const pathCmp = a.path.localeCompare(b.path); const pathCmp = a.path.localeCompare(b.path);
if (pathCmp !== 0) return pathCmp; if (pathCmp !== 0) return pathCmp;
@@ -143640,7 +143661,61 @@ function buildThreadBlocks(threads, filePatchMap, reviewId) {
} }
threadBlocks.push({ path: thread.path, lineRange, content: block }); threadBlocks.push({ path: thread.path, lineRange, content: block });
} }
return { threadBlocks, reviewer }; return threadBlocks;
}
async function getReviewThreads(input) {
const response = await input.octokit.graphql(REVIEW_THREADS_QUERY, {
owner: input.owner,
name: input.name,
prNumber: input.pullNumber
});
const allThreads = response.repository?.pullRequest?.reviewThreads?.nodes ?? [];
const threadsForReview = allThreads.filter((thread) => {
if (!thread?.comments?.nodes) return false;
return thread.comments.nodes.some((c) => c?.pullRequestReview?.databaseId === input.reviewId);
});
if (!input.approvedBy) {
return threadsForReview;
}
const username = input.approvedBy;
return threadsForReview.filter((thread) => threadHasThumbsUpFrom(thread, username));
}
async function getReviewData(input) {
const [review, threads] = await Promise.all([
input.octokit.rest.pulls.getReview({
owner: input.owner,
repo: input.name,
pull_number: input.pullNumber,
review_id: input.reviewId
}),
getReviewThreads(input)
]);
const rawReviewBody = review.data.body;
const reviewBody = rawReviewBody ? stripExistingFooter(rawReviewBody) : "";
const reviewer = review.data.user?.login ?? "unknown";
if (threads.length === 0 && !reviewBody) return void 0;
let threadBlocks = [];
if (threads.length > 0) {
const prFilesResponse = await input.octokit.rest.pulls.listFiles({
owner: input.owner,
repo: input.name,
pull_number: input.pullNumber
});
const filePatchMap = /* @__PURE__ */ new Map();
for (const file2 of prFilesResponse.data) {
if (file2.patch) {
filePatchMap.set(file2.filename, parseFilePatches(file2.patch));
}
}
threadBlocks = buildThreadBlocks(threads, filePatchMap, input.reviewId);
}
const formatted = formatReviewThreads(threadBlocks, {
pullNumber: input.pullNumber,
reviewId: input.reviewId,
reviewer,
reviewBody
});
return { threadBlocks, reviewer, formatted };
} }
function GetReviewCommentsTool(ctx) { function GetReviewCommentsTool(ctx) {
return tool({ return tool({
@@ -143648,24 +143723,15 @@ function GetReviewCommentsTool(ctx) {
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.", 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, parameters: GetReviewComments,
execute: execute(async (params) => { execute: execute(async (params) => {
const response = await ctx.octokit.graphql(REVIEW_THREADS_QUERY, { const result = await getReviewData({
octokit: ctx.octokit,
owner: ctx.repo.owner, owner: ctx.repo.owner,
name: ctx.repo.name, name: ctx.repo.name,
prNumber: params.pull_number pullNumber: params.pull_number,
reviewId: params.review_id,
approvedBy: params.approved_by
}); });
const allThreads = response.repository?.pullRequest?.reviewThreads?.nodes ?? []; if (!result) {
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 { return {
review_id: params.review_id, review_id: params.review_id,
pull_number: params.pull_number, pull_number: params.pull_number,
@@ -143676,27 +143742,7 @@ function GetReviewCommentsTool(ctx) {
instructions: params.approved_by ? `no threads with \u{1F44D} from ${params.approved_by}` : "no threads found for this review" 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({ const { threadBlocks, reviewer, formatted } = result;
owner: ctx.repo.owner,
repo: ctx.repo.name,
pull_number: params.pull_number
});
const filePatchMap = /* @__PURE__ */ new Map();
for (const file2 of prFilesResponse.data) {
if (file2.patch) {
filePatchMap.set(file2.filename, parseFilePatches(file2.patch));
}
}
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; const tempDir = process.env.PULLFROG_TEMP_DIR;
if (!tempDir) { if (!tempDir) {
throw new Error("PULLFROG_TEMP_DIR not set"); throw new Error("PULLFROG_TEMP_DIR not set");
@@ -144481,7 +144527,7 @@ function computeModes() {
1. **CHECKOUT** - Determine whether to checkout the existing PR branch or create a new one: 1. **CHECKOUT** - Determine whether to checkout the existing PR branch or create a new one:
- **PR event, modifying the existing PR**: Call \`${ghPullfrogMcpName}/checkout_pr\` with the PR number to checkout the PR branch. - **PR event, modifying the existing PR**: Call \`${ghPullfrogMcpName}/checkout_pr\` with the PR number to checkout the PR branch.
- **PR event, but user wants a NEW branch/PR**: Create a new branch with \`git checkout -b pullfrog/branch-name\` via the \`${ghPullfrogMcpName}/git\` tool. - **PR event, but user wants a NEW branch/PR**: Create a new branch with \`git checkout -b pullfrog/branch-name\` via the \`${ghPullfrogMcpName}/git\` tool.
Branch names must be prefixed with "pullfrog/" and be specific enough to avoid collisions. Never commit directly to main/master/production. Branch names must be prefixed with "pullfrog/" and be specific enough to avoid collisions. Never commit directly to main/master/production.
2. **DEPENDENCIES** - ${dependencyInstallationStep} 2. **DEPENDENCIES** - ${dependencyInstallationStep}
@@ -144506,7 +144552,7 @@ function computeModes() {
10. **FINAL REPORT** - Call report_progress one final time ONLY if you haven't already included all the important information (PR links, branch links, summary) in a previous report_progress call. If you already called report_progress with complete information including PR links after creating the PR, you do NOT need to call it again. Only make a final call if you need to add missing information. When making the final call, ensure it includes: 10. **FINAL REPORT** - Call report_progress one final time ONLY if you haven't already included all the important information (PR links, branch links, summary) in a previous report_progress call. If you already called report_progress with complete information including PR links after creating the PR, you do NOT need to call it again. Only make a final call if you need to add missing information. When making the final call, ensure it includes:
- A summary of what was accomplished - A summary of what was accomplished
- Links to any artifacts created (PRs, branches, issues) - Links to any artifacts created (PRs, branches, issues)
- If you created a PR, ALWAYS include the PR link. e.g.: - If you created a PR, ALWAYS include the PR link. e.g.:
\`\`\`md \`\`\`md
[View PR \u2794](https://github.com/org/repo/pull/123) [View PR \u2794](https://github.com/org/repo/pull/123)
\`\`\` \`\`\`
@@ -144573,6 +144619,7 @@ Keep the progress comment extremely brief. The summary should be 1-2 sentences m
6. **SUBMIT** \u2014 Use ${ghPullfrogMcpName}/create_pull_request_review: 6. **SUBMIT** \u2014 Use ${ghPullfrogMcpName}/create_pull_request_review:
- \`body\`: The summary from step 5 - \`body\`: The summary from step 5
- \`comments\`: The inline comments from step 4 - \`comments\`: The inline comments from step 4
- \`approved\`: Set to \`true\` ONLY if the review contains no actionable feedback \u2014 neither inline comments nor actionable content in the body. An approval signals "no changes needed."
${permalinkTip} ${permalinkTip}
` `
@@ -144608,23 +144655,23 @@ ${permalinkTip}`
- \`failed_steps\`: which CI steps failed (e.g., "Step 6: Run tests") - \`failed_steps\`: which CI steps failed (e.g., "Step 6: Run tests")
2. **CHECKOUT AND ASSESS CAUSATION** - Use ${ghPullfrogMcpName}/checkout_pr to get the PR diff. BEFORE attempting any fix, you MUST determine if this PR caused the failure: 2. **CHECKOUT AND ASSESS CAUSATION** - Use ${ghPullfrogMcpName}/checkout_pr to get the PR diff. BEFORE attempting any fix, you MUST determine if this PR caused the failure:
**Ask yourself**: "Could the changes in this PR have caused this failure?" **Ask yourself**: "Could the changes in this PR have caused this failure?"
- Read the PR diff carefully - what files were modified? - Read the PR diff carefully - what files were modified?
- What is failing? (test file, module, assertion) - What is failing? (test file, module, assertion)
- Is there a PLAUSIBLE CONNECTION between the PR changes and the failure? - Is there a PLAUSIBLE CONNECTION between the PR changes and the failure?
**ABORT immediately if any of these are true:** **ABORT immediately if any of these are true:**
- The failing test/file was NOT touched by this PR AND doesn't depend on changed code - The failing test/file was NOT touched by this PR AND doesn't depend on changed code
- The error is infrastructure-related (network timeout, runner OOM, service unavailable) - The error is infrastructure-related (network timeout, runner OOM, service unavailable)
- The error is a flaky test that passes/fails randomly - The error is a flaky test that passes/fails randomly
- The error existed before this PR (pre-existing bug in main branch) - The error existed before this PR (pre-existing bug in main branch)
- The error is in a dependency update not introduced by this PR - The error is in a dependency update not introduced by this PR
**When aborting**, use ${ghPullfrogMcpName}/report_progress to explain: **When aborting**, use ${ghPullfrogMcpName}/report_progress to explain:
"This CI failure appears unrelated to the PR's changes. [Describe the failure]. [Explain why it's not caused by the PR]. No changes made." "This CI failure appears unrelated to the PR's changes. [Describe the failure]. [Explain why it's not caused by the PR]. No changes made."
**Only proceed** if there's a clear, logical connection between the PR changes and the failure. **Only proceed** if there's a clear, logical connection between the PR changes and the failure.
3. **UNDERSTAND HOW CI RUNS** - Read the workflow file to understand exactly what commands CI runs: 3. **UNDERSTAND HOW CI RUNS** - Read the workflow file to understand exactly what commands CI runs:
+33 -4
View File
@@ -1,11 +1,40 @@
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
exports[`formatReviewThreads > formats thread blocks with TOC and correct line numbers > content 1`] = ` exports[`getFormattedReviewThreads > formats body-only review > content 1`] = `
"# Review Threads (1) for PR #49 - Review 3485940013 by cursor "# Review Threads (0) for PR #64 - Review 3531000326 by pullfrog[bot]
## Review Body
This PR looks great. The retry logic is well-implemented and the tests are comprehensive.
---
"
`;
exports[`getFormattedReviewThreads > formats body-only review > toc 1`] = `""`;
exports[`getFormattedReviewThreads > formats thread blocks with TOC and correct line numbers > content 1`] = `
"# Review Threads (1) for PR #49 - Review 3485940013 by cursor[bot]
## TOC ## TOC
- .github/workflows/test.yml:7 → lines 9-36 - .github/workflows/test.yml:7 → lines 25-52
## Review Body
### This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on November 30
<details>
<summary>Details</summary>
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the [Cursor dashboard](https://www.cursor.com/dashboard?tab=bugbot) to activate Pro and start your 14-day free trial.
</details>
--- ---
@@ -39,4 +68,4 @@ LOCATIONS END -->
" "
`; `;
exports[`formatReviewThreads > formats thread blocks with TOC and correct line numbers > toc 1`] = `"- .github/workflows/test.yml:7 → lines 9-36"`; exports[`getFormattedReviewThreads > formats thread blocks with TOC and correct line numbers > toc 1`] = `"- .github/workflows/test.yml:7 → lines 25-52"`;
+18 -7
View File
@@ -15,6 +15,11 @@ export const CreatePullRequestReview = type({
"1-2 sentence high-level summary with urgency level, critical callouts, and feedback about code outside the diff. Specific feedback on diff lines goes in 'comments' array." "1-2 sentence high-level summary with urgency level, critical callouts, and feedback about code outside the diff. Specific feedback on diff lines goes in 'comments' array."
) )
.optional(), .optional(),
approved: type.boolean
.describe(
"Set to true to submit as an approval. ONLY when the review contains no actionable feedback — neither inline comments nor actionable content in the body. Defaults to false (comment-only review). Rejections are not supported."
)
.optional(),
commit_id: type.string commit_id: type.string
.describe("Optional SHA of the commit being reviewed. Defaults to latest.") .describe("Optional SHA of the commit being reviewed. Defaults to latest.")
.optional(), .optional(),
@@ -64,7 +69,7 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) {
" Commenting on files or lines outside the diff will cause GitHub API errors." + " Commenting on files or lines outside the diff will cause GitHub API errors." +
" Put feedback about code outside the diff in 'body' instead.", " Put feedback about code outside the diff in 'body' instead.",
parameters: CreatePullRequestReview, parameters: CreatePullRequestReview,
execute: execute(async ({ pull_number, body, commit_id, comments = [] }) => { execute: execute(async ({ pull_number, body, approved, commit_id, comments = [] }) => {
// set issue context (PRs are issues) // set issue context (PRs are issues)
ctx.toolState.issueNumber = pull_number; ctx.toolState.issueNumber = pull_number;
@@ -73,7 +78,7 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) {
owner: ctx.repo.owner, owner: ctx.repo.owner,
repo: ctx.repo.name, repo: ctx.repo.name,
pull_number, pull_number,
event: "COMMENT", event: approved ? "APPROVE" : "COMMENT",
}; };
if (body) params.body = body; if (body) params.body = body;
if (commit_id) { if (commit_id) {
@@ -120,11 +125,17 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) {
// build quick links footer and update the review body // build quick links footer and update the review body
// only include "Fix all" and "Fix 👍s" links if there are actual review comments // only include "Fix all" and "Fix 👍s" links if there are actual review comments
const customParts: string[] = []; const customParts: string[] = [];
if (comments.length > 0) { if (!approved) {
const apiUrl = getApiUrl(); if (comments.length > 0) {
const fixAllUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix&review_id=${reviewId}`; const apiUrl = getApiUrl();
const fixApprovedUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix-approved&review_id=${reviewId}`; const fixAllUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix&review_id=${reviewId}`;
customParts.push(`[Fix all ➔](${fixAllUrl})`, `[Fix 👍s ➔](${fixApprovedUrl})`); const fixApprovedUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix-approved&review_id=${reviewId}`;
customParts.push(`[Fix all ➔](${fixAllUrl})`, `[Fix 👍s ➔](${fixApprovedUrl})`);
} else if (body) {
const apiUrl = getApiUrl();
const fixUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix&review_id=${reviewId}`;
customParts.push(`[Fix it ➔](${fixUrl})`);
}
} }
const footer = buildPullfrogFooter({ const footer = buildPullfrogFooter({
+22 -39
View File
@@ -1,60 +1,43 @@
import { Octokit } from "@octokit/rest"; import { Octokit } from "@octokit/rest";
import { describe, expect, it } from "vitest"; import { describe, expect, it } from "vitest";
import { acquireNewToken } from "../utils/github.ts"; import { acquireNewToken } from "../utils/github.ts";
import { import { getReviewData } from "./reviewComments.ts";
buildThreadBlocks,
formatReviewThreads,
type ParsedHunk,
parseFilePatches,
REVIEW_THREADS_QUERY,
type ReviewThread,
type ReviewThreadsQueryResponse,
} from "./reviewComments.ts";
async function getToken(): Promise<string> { async function getToken(): Promise<string> {
// prefer explicit GH_TOKEN, fall back to acquiring one via GitHub App credentials
if (process.env.GH_TOKEN) return process.env.GH_TOKEN; if (process.env.GH_TOKEN) return process.env.GH_TOKEN;
return await acquireNewToken(); return await acquireNewToken();
} }
describe("formatReviewThreads", () => { describe("getFormattedReviewThreads", () => {
it("formats thread blocks with TOC and correct line numbers", { timeout: 30000 }, async () => { it("formats thread blocks with TOC and correct line numbers", { timeout: 30000 }, async () => {
const token = await getToken(); const token = await getToken();
const octokit = new Octokit({ auth: token }); const octokit = new Octokit({ auth: token });
const pullNumber = 49;
const reviewId = 3485940013;
// fetch review threads via GraphQL const { formatted } = (await getReviewData({
const response = await octokit.graphql<ReviewThreadsQueryResponse>(REVIEW_THREADS_QUERY, { octokit,
owner: "pullfrog", owner: "pullfrog",
name: "scratch", name: "scratch",
prNumber: pullNumber, pullNumber: 49,
}); reviewId: 3485940013,
}))!;
const allThreads = response.repository?.pullRequest?.reviewThreads?.nodes ?? []; expect(formatted.toc).toMatchSnapshot("toc");
const threadsForReview = allThreads.filter((thread): thread is ReviewThread => { expect(formatted.content).toMatchSnapshot("content");
if (!thread?.comments?.nodes) return false; });
return thread.comments.nodes.some((c) => c?.pullRequestReview?.databaseId === reviewId);
});
// fetch file patches it("formats body-only review", { timeout: 30000 }, async () => {
const prFilesResponse = await octokit.rest.pulls.listFiles({ const token = await getToken();
const octokit = new Octokit({ auth: token });
const { formatted } = (await getReviewData({
octokit,
owner: "pullfrog", owner: "pullfrog",
repo: "scratch", name: "scratch",
pull_number: pullNumber, pullNumber: 64,
}); reviewId: 3531000326,
const filePatchMap = new Map<string, ParsedHunk[]>(); }))!;
for (const file of prFilesResponse.data) {
if (file.patch) {
filePatchMap.set(file.filename, parseFilePatches(file.patch));
}
}
// build and format expect(formatted.toc).toMatchSnapshot("toc");
const { threadBlocks, reviewer } = buildThreadBlocks(threadsForReview, filePatchMap, reviewId); expect(formatted.content).toMatchSnapshot("content");
const result = formatReviewThreads(threadBlocks, { pullNumber, reviewId, reviewer });
expect(result.toc).toMatchSnapshot("toc");
expect(result.content).toMatchSnapshot("content");
}); });
}); });
+120 -63
View File
@@ -1,6 +1,8 @@
import { writeFileSync } from "node:fs"; import { writeFileSync } from "node:fs";
import { join } from "node:path"; import { join } from "node:path";
import type { Octokit } from "@octokit/rest";
import { type } from "arktype"; import { type } from "arktype";
import { stripExistingFooter } from "../utils/buildPullfrogFooter.ts";
import { log } from "../utils/log.ts"; import { log } from "../utils/log.ts";
import type { ToolContext } from "./server.ts"; import type { ToolContext } from "./server.ts";
import { execute, tool } from "./shared.ts"; import { execute, tool } from "./shared.ts";
@@ -94,6 +96,16 @@ export type ReviewThreadsQueryResponse = {
} | null; } | null;
}; };
export function countLines(str: string): number {
let count = 1;
let index = -1;
// biome-ignore lint/suspicious/noAssignInExpressions: assignment in while condition is intentional for indexOf loop pattern
while ((index = str.indexOf("\n", index + 1)) !== -1) {
count++;
}
return count;
}
// extract exactly the commented line range from diffHunk, plus context // extract exactly the commented line range from diffHunk, plus context
const CONTEXT_PADDING = 3; const CONTEXT_PADDING = 3;
@@ -290,8 +302,8 @@ function hasThumbsUpFrom(comment: ReviewThreadComment, username: string): boolea
if (!comment.reactionGroups) return false; if (!comment.reactionGroups) return false;
const thumbsUp = comment.reactionGroups.find((g) => g.content === "THUMBS_UP"); const thumbsUp = comment.reactionGroups.find((g) => g.content === "THUMBS_UP");
if (!thumbsUp?.reactors?.nodes) return false; if (!thumbsUp?.reactors?.nodes) return false;
const usernameNeedle = username.toLowerCase(); const needle = username.toLowerCase();
return thumbsUp.reactors.nodes.some((r) => r?.login?.toLowerCase() === usernameNeedle); return thumbsUp.reactors.nodes.some((r) => r?.login?.toLowerCase() === needle);
} }
function threadHasThumbsUpFrom(thread: ReviewThread, username: string): boolean { function threadHasThumbsUpFrom(thread: ReviewThread, username: string): boolean {
@@ -305,19 +317,26 @@ function threadHasThumbsUpFrom(thread: ReviewThread, username: string): boolean
*/ */
export function formatReviewThreads( export function formatReviewThreads(
threadBlocks: Array<{ path: string; lineRange: string; content: string[] }>, threadBlocks: Array<{ path: string; lineRange: string; content: string[] }>,
header: { pullNumber: number; reviewId: number; reviewer: string } header: { pullNumber: number; reviewId: number; reviewer: string; reviewBody?: string }
) { ) {
// header section takes: title (1) + blank (1) + "## TOC" (1) + blank (1) + N TOC entries + blank (1) + "---" (1) + blank (1) // header section takes: title (1) + blank (1) + "## TOC" (1) + blank (1) + N TOC entries + blank (1) + "---" (1) + blank (1)
const tocHeaderLines = 4; const tocHeaderLines = 4;
const tocFooterLines = 3; const tocFooterLines = 3;
let currentLine = tocHeaderLines + threadBlocks.length + tocFooterLines + 1; let currentLine = tocHeaderLines + threadBlocks.length + tocFooterLines + 1;
// account for review body section if present
const reviewBodyLines: string[] = [];
if (header.reviewBody) {
reviewBodyLines.push("## Review Body", "", header.reviewBody, "");
currentLine += reviewBodyLines.reduce((sum, line) => sum + countLines(line), 0);
}
const tocEntries: string[] = []; const tocEntries: string[] = [];
const threadLines: string[] = []; const threadLines: string[] = [];
for (const block of threadBlocks) { for (const block of threadBlocks) {
const startLine = currentLine; const startLine = currentLine;
const actualLineCount = block.content.reduce((sum, line) => sum + line.split("\n").length, 0); const actualLineCount = block.content.reduce((sum, line) => sum + countLines(line), 0);
const endLine = currentLine + actualLineCount - 1; const endLine = currentLine + actualLineCount - 1;
tocEntries.push(`- ${block.path}:${block.lineRange} → lines ${startLine}-${endLine}`); tocEntries.push(`- ${block.path}:${block.lineRange} → lines ${startLine}-${endLine}`);
threadLines.push(...block.content); threadLines.push(...block.content);
@@ -329,10 +348,13 @@ export function formatReviewThreads(
`# Review Threads (${threadBlocks.length}) for PR #${header.pullNumber} - Review ${header.reviewId} by ${header.reviewer}` `# Review Threads (${threadBlocks.length}) for PR #${header.pullNumber} - Review ${header.reviewId} by ${header.reviewer}`
); );
lines.push(""); lines.push("");
lines.push("## TOC"); if (threadBlocks.length > 0) {
lines.push(""); lines.push("## TOC");
lines.push(...tocEntries); lines.push("");
lines.push(""); lines.push(...tocEntries);
lines.push("");
}
lines.push(...reviewBodyLines);
lines.push("---"); lines.push("---");
lines.push(""); lines.push("");
lines.push(...threadLines); lines.push(...threadLines);
@@ -352,12 +374,6 @@ export function buildThreadBlocks(
filePatchMap: Map<string, ParsedHunk[]>, filePatchMap: Map<string, ParsedHunk[]>,
reviewId: number 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 // sort threads by file path, then by line number
threads.sort((a, b) => { threads.sort((a, b) => {
const pathCmp = a.path.localeCompare(b.path); const pathCmp = a.path.localeCompare(b.path);
@@ -439,7 +455,89 @@ export function buildThreadBlocks(
threadBlocks.push({ path: thread.path, lineRange, content: block }); threadBlocks.push({ path: thread.path, lineRange, content: block });
} }
return { threadBlocks, reviewer }; return threadBlocks;
}
async function getReviewThreads(input: GetReviewDataInput) {
const response = await input.octokit.graphql<ReviewThreadsQueryResponse>(REVIEW_THREADS_QUERY, {
owner: input.owner,
name: input.name,
prNumber: input.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 === input.reviewId);
});
if (!input.approvedBy) {
return threadsForReview;
}
const username = input.approvedBy;
return threadsForReview.filter((thread) => threadHasThumbsUpFrom(thread, username));
}
interface GetReviewDataInput {
octokit: Octokit;
owner: string;
name: string;
pullNumber: number;
reviewId: number;
approvedBy?: string | undefined;
}
export async function getReviewData(input: GetReviewDataInput): Promise<
| {
threadBlocks: Array<{ path: string; lineRange: string; content: string[] }>;
reviewer: string;
formatted: { toc: string; content: string };
}
| undefined
> {
const [review, threads] = await Promise.all([
input.octokit.rest.pulls.getReview({
owner: input.owner,
repo: input.name,
pull_number: input.pullNumber,
review_id: input.reviewId,
}),
getReviewThreads(input),
]);
const rawReviewBody = review.data.body;
const reviewBody = rawReviewBody ? stripExistingFooter(rawReviewBody) : "";
const reviewer = review.data.user?.login ?? "unknown";
if (threads.length === 0 && !reviewBody) return undefined;
let threadBlocks: Array<{ path: string; lineRange: string; content: string[] }> = [];
if (threads.length > 0) {
const prFilesResponse = await input.octokit.rest.pulls.listFiles({
owner: input.owner,
repo: input.name,
pull_number: input.pullNumber,
});
const filePatchMap = new Map<string, ParsedHunk[]>();
for (const file of prFilesResponse.data) {
if (file.patch) {
filePatchMap.set(file.filename, parseFilePatches(file.patch));
}
}
threadBlocks = buildThreadBlocks(threads, filePatchMap, input.reviewId);
}
const formatted = formatReviewThreads(threadBlocks, {
pullNumber: input.pullNumber,
reviewId: input.reviewId,
reviewer,
reviewBody,
});
return { threadBlocks, reviewer, formatted };
} }
export function GetReviewCommentsTool(ctx: ToolContext) { export function GetReviewCommentsTool(ctx: ToolContext) {
@@ -451,31 +549,16 @@ export function GetReviewCommentsTool(ctx: ToolContext) {
"Returns a TOC and commentsPath pointing to a markdown file with full comment details.", "Returns a TOC and commentsPath pointing to a markdown file with full comment details.",
parameters: GetReviewComments, parameters: GetReviewComments,
execute: execute(async (params) => { execute: execute(async (params) => {
// fetch all review threads for the PR via GraphQL const result = await getReviewData({
const response = await ctx.octokit.graphql<ReviewThreadsQueryResponse>(REVIEW_THREADS_QUERY, { octokit: ctx.octokit,
owner: ctx.repo.owner, owner: ctx.repo.owner,
name: ctx.repo.name, name: ctx.repo.name,
prNumber: params.pull_number, pullNumber: params.pull_number,
reviewId: params.review_id,
approvedBy: params.approved_by,
}); });
const allThreads = response.repository?.pullRequest?.reviewThreads?.nodes ?? []; if (!result) {
// 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
);
});
// filter by approved_by if specified
if (params.approved_by) {
threadsForReview = threadsForReview.filter((thread) =>
threadHasThumbsUpFrom(thread, params.approved_by as string)
);
}
if (threadsForReview.length === 0) {
return { return {
review_id: params.review_id, review_id: params.review_id,
pull_number: params.pull_number, pull_number: params.pull_number,
@@ -489,34 +572,8 @@ export function GetReviewCommentsTool(ctx: ToolContext) {
}; };
} }
// fetch full file patches for better multi-hunk context const { threadBlocks, reviewer, formatted } = result;
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<string, ParsedHunk[]>();
for (const file of prFilesResponse.data) {
if (file.patch) {
filePatchMap.set(file.filename, parseFilePatches(file.patch));
}
}
// build thread blocks
const { threadBlocks, reviewer } = buildThreadBlocks(
threadsForReview,
filePatchMap,
params.review_id
);
// 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; const tempDir = process.env.PULLFROG_TEMP_DIR;
if (!tempDir) { if (!tempDir) {
throw new Error("PULLFROG_TEMP_DIR not set"); throw new Error("PULLFROG_TEMP_DIR not set");
+8 -7
View File
@@ -32,7 +32,7 @@ export function computeModes(): Mode[] {
1. **CHECKOUT** - Determine whether to checkout the existing PR branch or create a new one: 1. **CHECKOUT** - Determine whether to checkout the existing PR branch or create a new one:
- **PR event, modifying the existing PR**: Call \`${ghPullfrogMcpName}/checkout_pr\` with the PR number to checkout the PR branch. - **PR event, modifying the existing PR**: Call \`${ghPullfrogMcpName}/checkout_pr\` with the PR number to checkout the PR branch.
- **PR event, but user wants a NEW branch/PR**: Create a new branch with \`git checkout -b pullfrog/branch-name\` via the \`${ghPullfrogMcpName}/git\` tool. - **PR event, but user wants a NEW branch/PR**: Create a new branch with \`git checkout -b pullfrog/branch-name\` via the \`${ghPullfrogMcpName}/git\` tool.
Branch names must be prefixed with "pullfrog/" and be specific enough to avoid collisions. Never commit directly to main/master/production. Branch names must be prefixed with "pullfrog/" and be specific enough to avoid collisions. Never commit directly to main/master/production.
2. **DEPENDENCIES** - ${dependencyInstallationStep} 2. **DEPENDENCIES** - ${dependencyInstallationStep}
@@ -57,7 +57,7 @@ export function computeModes(): Mode[] {
10. **FINAL REPORT** - Call report_progress one final time ONLY if you haven't already included all the important information (PR links, branch links, summary) in a previous report_progress call. If you already called report_progress with complete information including PR links after creating the PR, you do NOT need to call it again. Only make a final call if you need to add missing information. When making the final call, ensure it includes: 10. **FINAL REPORT** - Call report_progress one final time ONLY if you haven't already included all the important information (PR links, branch links, summary) in a previous report_progress call. If you already called report_progress with complete information including PR links after creating the PR, you do NOT need to call it again. Only make a final call if you need to add missing information. When making the final call, ensure it includes:
- A summary of what was accomplished - A summary of what was accomplished
- Links to any artifacts created (PRs, branches, issues) - Links to any artifacts created (PRs, branches, issues)
- If you created a PR, ALWAYS include the PR link. e.g.: - If you created a PR, ALWAYS include the PR link. e.g.:
\`\`\`md \`\`\`md
[View PR ➔](https://github.com/org/repo/pull/123) [View PR ➔](https://github.com/org/repo/pull/123)
\`\`\` \`\`\`
@@ -126,6 +126,7 @@ Keep the progress comment extremely brief. The summary should be 1-2 sentences m
6. **SUBMIT** — Use ${ghPullfrogMcpName}/create_pull_request_review: 6. **SUBMIT** — Use ${ghPullfrogMcpName}/create_pull_request_review:
- \`body\`: The summary from step 5 - \`body\`: The summary from step 5
- \`comments\`: The inline comments from step 4 - \`comments\`: The inline comments from step 4
- \`approved\`: Set to \`true\` ONLY if the review contains no actionable feedback — neither inline comments nor actionable content in the body. An approval signals "no changes needed."
${permalinkTip} ${permalinkTip}
`, `,
@@ -163,23 +164,23 @@ ${permalinkTip}`,
- \`failed_steps\`: which CI steps failed (e.g., "Step 6: Run tests") - \`failed_steps\`: which CI steps failed (e.g., "Step 6: Run tests")
2. **CHECKOUT AND ASSESS CAUSATION** - Use ${ghPullfrogMcpName}/checkout_pr to get the PR diff. BEFORE attempting any fix, you MUST determine if this PR caused the failure: 2. **CHECKOUT AND ASSESS CAUSATION** - Use ${ghPullfrogMcpName}/checkout_pr to get the PR diff. BEFORE attempting any fix, you MUST determine if this PR caused the failure:
**Ask yourself**: "Could the changes in this PR have caused this failure?" **Ask yourself**: "Could the changes in this PR have caused this failure?"
- Read the PR diff carefully - what files were modified? - Read the PR diff carefully - what files were modified?
- What is failing? (test file, module, assertion) - What is failing? (test file, module, assertion)
- Is there a PLAUSIBLE CONNECTION between the PR changes and the failure? - Is there a PLAUSIBLE CONNECTION between the PR changes and the failure?
**ABORT immediately if any of these are true:** **ABORT immediately if any of these are true:**
- The failing test/file was NOT touched by this PR AND doesn't depend on changed code - The failing test/file was NOT touched by this PR AND doesn't depend on changed code
- The error is infrastructure-related (network timeout, runner OOM, service unavailable) - The error is infrastructure-related (network timeout, runner OOM, service unavailable)
- The error is a flaky test that passes/fails randomly - The error is a flaky test that passes/fails randomly
- The error existed before this PR (pre-existing bug in main branch) - The error existed before this PR (pre-existing bug in main branch)
- The error is in a dependency update not introduced by this PR - The error is in a dependency update not introduced by this PR
**When aborting**, use ${ghPullfrogMcpName}/report_progress to explain: **When aborting**, use ${ghPullfrogMcpName}/report_progress to explain:
"This CI failure appears unrelated to the PR's changes. [Describe the failure]. [Explain why it's not caused by the PR]. No changes made." "This CI failure appears unrelated to the PR's changes. [Describe the failure]. [Explain why it's not caused by the PR]. No changes made."
**Only proceed** if there's a clear, logical connection between the PR changes and the failure. **Only proceed** if there's a clear, logical connection between the PR changes and the failure.
3. **UNDERSTAND HOW CI RUNS** - Read the workflow file to understand exactly what commands CI runs: 3. **UNDERSTAND HOW CI RUNS** - Read the workflow file to understand exactly what commands CI runs: