From c518e8b6fda7dd7c0c845b235f945b4bd1b8048a Mon Sep 17 00:00:00 2001 From: Colin McDonnell Date: Mon, 22 Dec 2025 15:53:11 -0800 Subject: [PATCH] Add retrying. Improve diff format --- entry | 87 +++++++++++++++++++++++++++++++++++++++---------- mcp/checkout.ts | 87 +++++++++++++++++++++++++++++++++++++++++++++---- mcp/pr.ts | 2 +- mcp/review.ts | 9 +++-- mcp/server.ts | 4 +-- modes.ts | 16 +++++---- utils/retry.ts | 47 ++++++++++++++++++++++++++ 7 files changed, 213 insertions(+), 39 deletions(-) create mode 100644 utils/retry.ts diff --git a/entry b/entry index 818380b..760197c 100755 --- a/entry +++ b/entry @@ -92638,10 +92638,9 @@ ${disableProgressComment ? "" : ` - Is the approach sound? If not, focus on the approach first. Don't waste time on implementation details if the approach is wrong. - What bugs, edge cases, or security issues exist? -4. **DRAFT** - Mentally list all comments you would make. For each: - - Note the file path (relative to repo root, e.g., \`packages/core/src/utils.ts\`) - - Note the line number from the diff (use NEW file line number - shown after \`+\` in hunk headers like \`@@ -10,5 +12,8 @@\` means new file starts at line 12) - - Draft the comment text +4. **DRAFT** - For each inline comment, find the line in the diff. Each code line shows: \`OLD | NEW | TYPE | code\` + - For added lines (\`+\`) or context lines, use the NEW line number (second column) + - For removed lines (\`-\`), use the OLD line number (first column) with \`side: "LEFT"\` 5. **SELF-CRITIQUE** - Before submitting, review your draft: - DO NOT NITPICK. Do not comment on minor formatting changes, changes to playground/scratch files, lack of docs/docsstrings, or small changes that seem irrelevant. Assume these things are intentional by the PR author. @@ -92650,12 +92649,15 @@ ${disableProgressComment ? "" : ` - For issues appearing in multiple places, keep only the FIRST occurrence and reference others (e.g., "also at lines X, Y") 6. **SUBMIT** - Use ${ghPullfrogMcpName}/create_pull_request_review with: - - \`comments\`: Array of all inline comments with file paths and line numbers - - \`body\`: 1-3 sentence summary with urgency level (e.g., "minor suggestions" vs "blocking issues") and any critical callouts (e.g., API key exposure) +- \`comments\`: Array of all inline comments with file paths and line numbers +- \`body\`: Everything else. Aim for a 1-3 sentence summary of the urgency level (e.g., "minor suggestions" vs "blocking issues") and any critical callouts (e.g., API key exposure). It can be longer if there are concerns that do not lend themselves to inline comments. + **CRITICAL RULES** +- ALL feedback goes in the ONE create_pull_request_review call. Do not create separate comments. +- Inline \`comments\` can only be placed on lines within diff hunks. For feedback about code outside the diff (e.g., "function X has the same issue"), include it in the \`body\`. +- Cross-cutting concerns that don't fit on a specific line go in the \`body\`, not in a separate comment. - 95%+ of review content should be in inline \`comments\` array, not the \`body\` -- Only comment on lines that appear in the diff - GitHub will reject comments on unchanged lines - Do not leave complimentary comments just to be nice - Do not leave comments that are not actionable ` @@ -123017,6 +123019,55 @@ function $(cmd, args3, options) { } // mcp/checkout.ts +function formatFilesWithLineNumbers(files) { + const output = []; + for (const file of files) { + output.push(`diff --git a/${file.filename} b/${file.filename}`); + output.push(`--- a/${file.filename}`); + output.push(`+++ b/${file.filename}`); + if (!file.patch) { + output.push("(binary file or no changes)"); + output.push(""); + continue; + } + const lines = file.patch.split("\n"); + let oldLine = 0; + let newLine = 0; + for (const line of lines) { + const hunkMatch = line.match(/^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/); + if (hunkMatch) { + oldLine = parseInt(hunkMatch[1], 10); + newLine = parseInt(hunkMatch[2], 10); + output.push(line); + continue; + } + const changeType = line[0] || " "; + const code = line.slice(1); + if (changeType === "-") { + output.push(`${padNum(oldLine)} | | - | ${code}`); + oldLine++; + } else if (changeType === "+") { + output.push(` | ${padNum(newLine)} | + | ${code}`); + newLine++; + } else if (changeType === " " || changeType === "\\") { + if (changeType === "\\") { + output.push(line); + } else { + output.push(`${padNum(oldLine)} | ${padNum(newLine)} | | ${code}`); + oldLine++; + newLine++; + } + } else { + output.push(line); + } + } + output.push(""); + } + return output.join("\n"); +} +function padNum(n) { + return n.toString().padStart(4, " "); +} var CheckoutPr = type({ pull_number: type.number.describe("the pull request number to checkout") }); @@ -123100,13 +123151,16 @@ function CheckoutPrTool(ctx) { if (!headRepo) { throw new Error(`PR #${pull_number} source repository was deleted`); } - const diffResponse = await ctx.octokit.rest.pulls.get({ + const filesResponse = await ctx.octokit.rest.pulls.listFiles({ owner: ctx.owner, repo: ctx.name, pull_number, - mediaType: { format: "diff" } + per_page: 100 }); - const diffContent = diffResponse.data; + const diffContent = formatFilesWithLineNumbers(filesResponse.data); + const diffPreview = diffContent.split("\n").slice(0, 100).join("\n"); + log.debug(`formatted diff preview (first 100 lines): +${diffPreview}`); const tempDir = process.env.PULLFROG_TEMP_DIR; if (!tempDir) { throw new Error( @@ -124399,7 +124453,7 @@ function buildPrBodyWithFooter(ctx, body) { const bodyWithoutFooter = stripExistingFooter(body); return `${bodyWithoutFooter}${footer}`; } -function PullRequestTool(ctx) { +function CreatePullRequestTool(ctx) { return tool({ name: "create_pull_request", description: "Create a pull request from the current branch", @@ -124476,24 +124530,23 @@ function PullRequestInfoTool(ctx) { var CreatePullRequestReview = type({ pull_number: type.number.describe("The pull request number to review"), body: type.string.describe( - "1-2 sentence high-level summary ONLY. Include urgency level and critical callouts (e.g., API key leak). ALL specific feedback MUST go in 'comments' array instead." + "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(), commit_id: type.string.describe("Optional SHA of the commit being reviewed. Defaults to latest.").optional(), comments: type({ path: type.string.describe("The file path to comment on (relative to repo root)"), line: type.number.describe( - "The line number in the file (use line numbers from the diff - usually the RIGHT side/new code)" + "Line number from the diff. Each code line shows 'OLD | NEW | TYPE | code'. Use NEW (second column) for added/context lines, OLD (first column) for removed lines." ), side: type.enumerated("LEFT", "RIGHT").describe( - "Side of the diff: LEFT (old code) or RIGHT (new code). Defaults to RIGHT if not provided." + "Side of the diff: LEFT (old code, lines starting with -) or RIGHT (new code, lines starting with + or unchanged). Defaults to RIGHT." ).optional(), body: type.string.describe( "The comment text for this specific line. For issues appearing multiple times, comment on the first occurrence and reference others." ), start_line: type.number.describe("Start line for multi-line comments (optional, for commenting on ranges)").optional() }).array().describe( - // FORK PR NOTE: checkout_pr returns the diff via GitHub API - use that for line numbers - "PRIMARY location for ALL feedback. 95%+ of review content should be here. Use the diff returned from checkout_pr to find correct line numbers (RIGHT side for new code, LEFT for old)." + "Inline comments on lines within diff hunks. Feedback about code outside the diff goes in 'body' instead." ).optional() }); function CreatePullRequestReviewTool(ctx) { @@ -124776,7 +124829,7 @@ async function startMcpHttpServer(ctx) { IssueInfoTool(ctx), GetIssueCommentsTool(ctx), GetIssueEventsTool(ctx), - PullRequestTool(ctx), + CreatePullRequestTool(ctx), CreatePullRequestReviewTool(ctx), PullRequestInfoTool(ctx), CheckoutPrTool(ctx), diff --git a/mcp/checkout.ts b/mcp/checkout.ts index 8e32a38..16f5bfe 100644 --- a/mcp/checkout.ts +++ b/mcp/checkout.ts @@ -1,12 +1,85 @@ import { writeFileSync } from "node:fs"; import { join } from "node:path"; -import type { Octokit } from "@octokit/rest"; +import type { Octokit, RestEndpointMethodTypes } from "@octokit/rest"; import { type } from "arktype"; import type { ToolContext } from "../main.ts"; import { log } from "../utils/cli.ts"; import { $ } from "../utils/shell.ts"; import { execute, tool } from "./shared.ts"; +type PullFile = RestEndpointMethodTypes["pulls"]["listFiles"]["response"]["data"][number]; + +/** + * 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 + */ +export function formatFilesWithLineNumbers(files: PullFile[]): string { + const output: string[] = []; + + for (const file of files) { + // file header + output.push(`diff --git a/${file.filename} b/${file.filename}`); + output.push(`--- a/${file.filename}`); + output.push(`+++ b/${file.filename}`); + + if (!file.patch) { + output.push("(binary file or no changes)"); + output.push(""); + continue; + } + + // parse and format the patch with line numbers + const lines = file.patch.split("\n"); + let oldLine = 0; + let newLine = 0; + + for (const line of lines) { + // hunk header: @@ -OLD,COUNT +NEW,COUNT @@ optional context + const hunkMatch = line.match(/^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/); + if (hunkMatch) { + oldLine = parseInt(hunkMatch[1], 10); + newLine = parseInt(hunkMatch[2], 10); + output.push(line); // pass through unchanged + continue; + } + + // code lines within hunks + const changeType = line[0] || " "; + const code = line.slice(1); + + if (changeType === "-") { + // removed line: show old line number, no new line number + output.push(`${padNum(oldLine)} | | - | ${code}`); + oldLine++; + } else if (changeType === "+") { + // added line: no old line number, show new line number + output.push(` | ${padNum(newLine)} | + | ${code}`); + newLine++; + } else if (changeType === " " || changeType === "\\") { + // context line or "\ No newline at end of file" + if (changeType === "\\") { + output.push(line); // pass through as-is + } else { + output.push(`${padNum(oldLine)} | ${padNum(newLine)} | | ${code}`); + oldLine++; + newLine++; + } + } else { + // unknown line type, pass through + output.push(line); + } + } + output.push(""); // blank line between files + } + + return output.join("\n"); +} + +function padNum(n: number): string { + return n.toString().padStart(4, " "); +} + export const CheckoutPr = type({ pull_number: type.number.describe("the pull request number to checkout"), }); @@ -170,16 +243,16 @@ export function CheckoutPrTool(ctx: ToolContext) { throw new Error(`PR #${pull_number} source repository was deleted`); } - // fetch PR diff via API (authoritative source - not affected by main advancing) - const diffResponse = await ctx.octokit.rest.pulls.get({ + // fetch PR files and format with line numbers + const filesResponse = await ctx.octokit.rest.pulls.listFiles({ owner: ctx.owner, repo: ctx.name, pull_number, - mediaType: { format: "diff" }, + per_page: 100, }); - - // write diff to file for grep access - const diffContent = diffResponse.data as unknown as string; + const diffContent = formatFilesWithLineNumbers(filesResponse.data); + const diffPreview = diffContent.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) { throw new Error( diff --git a/mcp/pr.ts b/mcp/pr.ts index ddac169..48ba397 100644 --- a/mcp/pr.ts +++ b/mcp/pr.ts @@ -29,7 +29,7 @@ function buildPrBodyWithFooter(ctx: ToolContext, body: string): string { return `${bodyWithoutFooter}${footer}`; } -export function PullRequestTool(ctx: ToolContext) { +export function CreatePullRequestTool(ctx: ToolContext) { return tool({ name: "create_pull_request", description: "Create a pull request from the current branch", diff --git a/mcp/review.ts b/mcp/review.ts index 2e10a5f..88bdf77 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -11,7 +11,7 @@ export const CreatePullRequestReview = type({ pull_number: type.number.describe("The pull request number to review"), body: type.string .describe( - "1-2 sentence high-level summary ONLY. Include urgency level and critical callouts (e.g., API key leak). ALL specific feedback MUST go in 'comments' array instead." + "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(), commit_id: type.string @@ -20,12 +20,12 @@ export const CreatePullRequestReview = type({ comments: type({ path: type.string.describe("The file path to comment on (relative to repo root)"), line: type.number.describe( - "The line number in the file (use line numbers from the diff - usually the RIGHT side/new code)" + "Line number from the diff. Each code line shows 'OLD | NEW | TYPE | code'. Use NEW (second column) for added/context lines, OLD (first column) for removed lines." ), side: type .enumerated("LEFT", "RIGHT") .describe( - "Side of the diff: LEFT (old code) or RIGHT (new code). Defaults to RIGHT if not provided." + "Side of the diff: LEFT (old code, lines starting with -) or RIGHT (new code, lines starting with + or unchanged). Defaults to RIGHT." ) .optional(), body: type.string.describe( @@ -37,8 +37,7 @@ export const CreatePullRequestReview = type({ }) .array() .describe( - // FORK PR NOTE: checkout_pr returns the diff via GitHub API - use that for line numbers - "PRIMARY location for ALL feedback. 95%+ of review content should be here. Use the diff returned from checkout_pr to find correct line numbers (RIGHT side for new code, LEFT for old)." + "Inline comments on lines within diff hunks. Feedback about code outside the diff goes in 'body' instead." ) .optional(), }); diff --git a/mcp/server.ts b/mcp/server.ts index 63010da..3979378 100644 --- a/mcp/server.ts +++ b/mcp/server.ts @@ -24,7 +24,7 @@ import { GetIssueCommentsTool } from "./issueComments.ts"; import { GetIssueEventsTool } from "./issueEvents.ts"; import { IssueInfoTool } from "./issueInfo.ts"; import { AddLabelsTool } from "./labels.ts"; -import { PullRequestTool } from "./pr.ts"; +import { CreatePullRequestTool } from "./pr.ts"; import { PullRequestInfoTool } from "./prInfo.ts"; import { CreatePullRequestReviewTool } from "./review.ts"; import { GetReviewCommentsTool, ListPullRequestReviewsTool } from "./reviewComments.ts"; @@ -83,7 +83,7 @@ export async function startMcpHttpServer( IssueInfoTool(ctx), GetIssueCommentsTool(ctx), GetIssueEventsTool(ctx), - PullRequestTool(ctx), + CreatePullRequestTool(ctx), CreatePullRequestReviewTool(ctx), PullRequestInfoTool(ctx), CheckoutPrTool(ctx), diff --git a/modes.ts b/modes.ts index 45fca21..b6e221e 100644 --- a/modes.ts +++ b/modes.ts @@ -110,10 +110,9 @@ ${ - Is the approach sound? If not, focus on the approach first. Don't waste time on implementation details if the approach is wrong. - What bugs, edge cases, or security issues exist? -4. **DRAFT** - Mentally list all comments you would make. For each: - - Note the file path (relative to repo root, e.g., \`packages/core/src/utils.ts\`) - - Note the line number from the diff (use NEW file line number - shown after \`+\` in hunk headers like \`@@ -10,5 +12,8 @@\` means new file starts at line 12) - - Draft the comment text +4. **DRAFT** - For each inline comment, find the line in the diff. Each code line shows: \`OLD | NEW | TYPE | code\` + - For added lines (\`+\`) or context lines, use the NEW line number (second column) + - For removed lines (\`-\`), use the OLD line number (first column) with \`side: "LEFT"\` 5. **SELF-CRITIQUE** - Before submitting, review your draft: - DO NOT NITPICK. Do not comment on minor formatting changes, changes to playground/scratch files, lack of docs/docsstrings, or small changes that seem irrelevant. Assume these things are intentional by the PR author. @@ -122,12 +121,15 @@ ${ - For issues appearing in multiple places, keep only the FIRST occurrence and reference others (e.g., "also at lines X, Y") 6. **SUBMIT** - Use ${ghPullfrogMcpName}/create_pull_request_review with: - - \`comments\`: Array of all inline comments with file paths and line numbers - - \`body\`: 1-3 sentence summary with urgency level (e.g., "minor suggestions" vs "blocking issues") and any critical callouts (e.g., API key exposure) +- \`comments\`: Array of all inline comments with file paths and line numbers +- \`body\`: Everything else. Aim for a 1-3 sentence summary of the urgency level (e.g., "minor suggestions" vs "blocking issues") and any critical callouts (e.g., API key exposure). It can be longer if there are concerns that do not lend themselves to inline comments. + **CRITICAL RULES** +- ALL feedback goes in the ONE create_pull_request_review call. Do not create separate comments. +- Inline \`comments\` can only be placed on lines within diff hunks. For feedback about code outside the diff (e.g., "function X has the same issue"), include it in the \`body\`. +- Cross-cutting concerns that don't fit on a specific line go in the \`body\`, not in a separate comment. - 95%+ of review content should be in inline \`comments\` array, not the \`body\` -- Only comment on lines that appear in the diff - GitHub will reject comments on unchanged lines - Do not leave complimentary comments just to be nice - Do not leave comments that are not actionable `, diff --git a/utils/retry.ts b/utils/retry.ts new file mode 100644 index 0000000..04859ff --- /dev/null +++ b/utils/retry.ts @@ -0,0 +1,47 @@ +import { log } from "./cli.ts"; + +export type RetryOptions = { + maxAttempts?: number; + delayMs?: number; + shouldRetry?: (error: unknown) => boolean; + label?: string; +}; + +const defaultShouldRetry = (error: unknown): boolean => { + if (!(error instanceof Error)) return false; + // retry on transient network errors + return ( + error.name === "AbortError" || + error.message.includes("fetch failed") || + error.message.includes("ECONNRESET") || + error.message.includes("ETIMEDOUT") + ); +}; + +export async function retry(fn: () => Promise, options: RetryOptions = {}): Promise { + const maxAttempts = options.maxAttempts ?? 3; + const delayMs = options.delayMs ?? 1000; + const shouldRetry = options.shouldRetry ?? defaultShouldRetry; + const label = options.label ?? "operation"; + + let lastError: unknown; + + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + try { + return await fn(); + } catch (error) { + lastError = error; + + if (attempt === maxAttempts || !shouldRetry(error)) { + throw error; + } + + const delay = delayMs * attempt; + log.warning(`ยป ${label} failed (attempt ${attempt}/${maxAttempts}), retrying in ${delay}ms...`); + await new Promise((resolve) => setTimeout(resolve, delay)); + } + } + + throw lastError; +} +