From c3c0794504ef63114d7391ba4116c7f40931d003 Mon Sep 17 00:00:00 2001 From: Colin McDonnell Date: Fri, 16 Jan 2026 19:36:35 +0000 Subject: [PATCH] Curate context and switch to file-based review comments --- entry | 264 ++++++++++++++++++++---------------------- external.ts | 19 +-- mcp/review.ts | 39 +++++-- mcp/reviewComments.ts | 245 +++++++++++++-------------------------- 4 files changed, 238 insertions(+), 329 deletions(-) diff --git a/entry b/entry index 6d8df94..19ccd8b 100755 --- a/entry +++ b/entry @@ -119135,15 +119135,18 @@ var CreatePullRequestReview = type({ comments: type({ path: type.string.describe("The file path to comment on (relative to repo root)"), line: type.number.describe( - "Line number from the diff. Each code line shows 'OLD | NEW | TYPE | CODE'. Use the NEW column (second column)." + "End line of the comment range (or the only line if no start_line). From diff format 'OLD | NEW | TYPE | CODE', use the NEW column." ), side: type.enumerated("LEFT", "RIGHT").describe( "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. When providing code suggestions, use GitHub's suggestion format with ```suggestion blocks to enable one-click apply. Only include explanatory text if the suggested code requires clarification." - ), - start_line: type.number.describe("Start line for multi-line comments (optional, for commenting on ranges)").optional() + body: type.string.describe("Explanatory comment text (optional if suggestion is provided)").optional(), + suggestion: type.string.describe( + "Full replacement code for the line range. Replaces the commented lines entirely - can be more, fewer, or same number of lines." + ).optional(), + start_line: type.number.describe( + "Start line for multi-line comments/suggestions. The range [start_line, line] defines which lines get replaced." + ).optional() }).array().describe( "Inline comments on lines within diff hunks. Feedback about code outside the diff goes in 'body' instead." ).optional() @@ -119151,7 +119154,7 @@ var CreatePullRequestReview = type({ function CreatePullRequestReviewTool(ctx) { return tool({ 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. When suggesting code changes in comments, use GitHub's suggestion format (```suggestion blocks) to enable one-click apply.", + 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' field to suggest a FULL REPLACEMENT of the associated line range. This will show up for users as a one-click apply suggestion. It doesn't need to be the same length as the original range, but the suggested code must work when applied.", parameters: CreatePullRequestReview, execute: execute(async ({ pull_number, body, commit_id, comments = [] }) => { ctx.toolState.prNumber = pull_number; @@ -119174,8 +119177,15 @@ function CreatePullRequestReviewTool(ctx) { } if (comments.length > 0) { params.comments = comments.map((comment) => { + let commentBody = comment.body || ""; + if (comment.suggestion !== void 0) { + const suggestionBlock = "```suggestion\n" + comment.suggestion + "\n```"; + commentBody = commentBody ? commentBody + "\n\n" + suggestionBlock : suggestionBlock; + } const reviewComment = { - ...comment + path: comment.path, + line: comment.line, + body: commentBody }; reviewComment.side = comment.side || "RIGHT"; if (comment.start_line) { @@ -119195,7 +119205,12 @@ function CreatePullRequestReviewTool(ctx) { const fixAllUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix&review_id=${reviewId}`; const fixApprovedUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix-approved&review_id=${reviewId}`; const footer = buildPullfrogFooter({ - workflowRun: { owner: ctx.repo.owner, repo: ctx.repo.name, runId: ctx.runId, jobId: ctx.jobId }, + workflowRun: { + owner: ctx.repo.owner, + repo: ctx.repo.name, + runId: ctx.runId, + jobId: ctx.jobId + }, customParts: [`[Fix all \u2794](${fixAllUrl})`, `[Fix \u{1F44D}s \u2794](${fixApprovedUrl})`] }); const updatedBody = (body || "") + footer; @@ -119220,109 +119235,82 @@ function CreatePullRequestReviewTool(ctx) { } // mcp/reviewComments.ts -var REVIEW_THREADS_QUERY = ` -query ($owner: String!, $repo: String!, $pullNumber: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $pullNumber) { - reviewThreads(first: 100) { - nodes { - diffSide - startDiffSide - comments(first: 100) { - nodes { - id - databaseId - body - path - line - startLine - url - author { - login - } - createdAt - updatedAt - pullRequestReview { - databaseId - } - replyTo { - databaseId - } - } - } - } - } - } - } -} -`; +import { writeFileSync as writeFileSync2 } from "node:fs"; +import { join as join4 } from "node:path"; var GetReviewComments = type({ pull_number: type.number.describe("The pull request number"), - review_id: type.number.describe("The review ID to get comments for") + 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 all review comments and their replies for a specific pull request review. Returns line-by-line comments that were left on specific code locations, including any threaded replies.", + description: "Get review comments for a pull request review, including diff 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.", parameters: GetReviewComments, - execute: execute(async ({ pull_number, review_id }) => { - const response = await ctx.octokit.graphql(REVIEW_THREADS_QUERY, { + execute: execute(async ({ pull_number, review_id, approved_by }) => { + const allComments = await ctx.octokit.paginate(ctx.octokit.rest.pulls.listReviewComments, { owner: ctx.repo.owner, repo: ctx.repo.name, - pullNumber: pull_number + pull_number }); - const pullRequest = response.repository?.pullRequest; - if (!pullRequest) { - return { - review_id, - pull_number, - comments: [], - count: 0 - }; - } - const threadNodes = pullRequest.reviewThreads?.nodes; - if (!threadNodes) { - return { - review_id, - pull_number, - comments: [], - count: 0 - }; - } - const allComments = []; - for (const thread of threadNodes) { - if (!thread?.comments?.nodes) continue; - const threadComments = thread.comments.nodes.filter( - (c) => c !== null - ); - if (threadComments.length === 0) continue; - const rootComment = threadComments.find((c) => c.replyTo === null); - if (!rootComment) continue; - const threadBelongsToReview = rootComment.pullRequestReview?.databaseId === review_id; - if (!threadBelongsToReview) continue; - for (const comment of threadComments) { - allComments.push({ - id: comment.databaseId, - body: comment.body, - path: comment.path, - line: comment.line, - start_line: comment.startLine, - side: thread.diffSide, - start_side: thread.startDiffSide, - user: comment.author?.login ?? null, - created_at: comment.createdAt, - updated_at: comment.updatedAt, - html_url: comment.url, - in_reply_to_id: comment.replyTo?.databaseId ?? null, - pull_request_review_id: comment.pullRequestReview?.databaseId ?? null + let reviewComments = allComments.filter((c) => c.pull_request_review_id === review_id); + if (approved_by) { + const approvedIds = /* @__PURE__ */ new Set(); + for (const comment of reviewComments) { + const reactions = await ctx.octokit.rest.reactions.listForPullRequestReviewComment({ + owner: ctx.repo.owner, + repo: ctx.repo.name, + comment_id: comment.id }); + const hasThumbsUp = reactions.data.some( + (r) => r.content === "+1" && r.user?.login === approved_by + ); + if (hasThumbsUp) approvedIds.add(comment.id); } + reviewComments = reviewComments.filter((c) => approvedIds.has(c.id)); } + if (reviewComments.length === 0) { + return { + review_id, + pull_number, + count: 0, + commentsPath: null, + message: approved_by ? `No comments with \u{1F44D} from ${approved_by}` : "No comments found for this review" + }; + } + const lines = []; + for (const comment of reviewComments) { + lines.push(`${"=".repeat(60)}`); + lines.push(`COMMENT #${comment.id} by @${comment.user?.login ?? "unknown"}`); + lines.push(`File: ${comment.path}:${comment.line ?? comment.original_line ?? "?"}`); + if (comment.in_reply_to_id) { + lines.push(`Reply to: #${comment.in_reply_to_id}`); + } + lines.push(""); + if (comment.diff_hunk) { + lines.push("```diff"); + lines.push(comment.diff_hunk); + lines.push("```"); + lines.push(""); + } + lines.push("Comment:"); + lines.push(comment.body); + lines.push(""); + } + const content = lines.join("\n"); + 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}.txt` : `review-${review_id}-comments.txt`; + const commentsPath = join4(tempDir, filename); + writeFileSync2(commentsPath, content); + log.debug(`wrote ${reviewComments.length} comments to ${commentsPath}`); return { review_id, pull_number, - comments: allComments, - count: allComments.length + count: reviewComments.length, + commentsPath }; }) }); @@ -119348,9 +119336,7 @@ function ListPullRequestReviewsTool(ctx) { body: review.body, state: review.state, user: review.user?.login, - commit_id: review.commit_id, - submitted_at: review.submitted_at, - html_url: review.html_url + submitted_at: review.submitted_at })), count: reviews.length }; @@ -119631,7 +119617,7 @@ import { spawn as spawn3 } from "child_process"; import { createInterface } from "readline"; import * as fs2 from "fs"; import { stat as statPromise, open } from "fs/promises"; -import { join as join4 } from "path"; +import { join as join6 } from "path"; import { homedir } from "os"; import { dirname, join as join22 } from "path"; import { cwd } from "process"; @@ -126333,7 +126319,7 @@ function shouldShowDebugMessage(message, filter) { return shouldShowDebugCategories(categories, filter); } function getClaudeConfigHomeDir() { - return process.env.CLAUDE_CONFIG_DIR ?? join4(homedir(), ".claude"); + return process.env.CLAUDE_CONFIG_DIR ?? join6(homedir(), ".claude"); } function isEnvTruthy(envVar) { if (!envVar) @@ -136238,7 +136224,7 @@ import { spawnSync as spawnSync2 } from "node:child_process"; import { chmodSync, createWriteStream as createWriteStream2, existsSync as existsSync4 } from "node:fs"; import { mkdtemp } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { join as join6 } from "node:path"; +import { join as join7 } from "node:path"; import { pipeline } from "node:stream/promises"; async function installFromNpmTarball(params) { let resolvedVersion = params.version; @@ -136262,7 +136248,7 @@ async function installFromNpmTarball(params) { } log.debug(`\xBB installing ${params.packageName}@${resolvedVersion}...`); const tempDir = process.env.PULLFROG_TEMP_DIR; - const tarballPath = join6(tempDir, "package.tgz"); + const tarballPath = join7(tempDir, "package.tgz"); const npmRegistry = process.env.NPM_REGISTRY || "https://registry.npmjs.org"; let tarballUrl; if (params.packageName.startsWith("@")) { @@ -136291,8 +136277,8 @@ async function installFromNpmTarball(params) { `Failed to extract tarball: ${extractResult.stderr || extractResult.stdout || "Unknown error"}` ); } - const extractedDir = join6(tempDir, "package"); - const cliPath = join6(extractedDir, params.executablePath); + const extractedDir = join7(tempDir, "package"); + const cliPath = join7(extractedDir, params.executablePath); if (!existsSync4(cliPath)) { throw new Error(`Executable not found in extracted package at ${cliPath}`); } @@ -136354,10 +136340,10 @@ async function installFromGithub(params) { const assetUrl = asset.browser_download_url; log.debug(`\xBB downloading asset from ${assetUrl}...`); const tempDirPrefix = `${params.owner}-${params.repo}-github-`; - const tempDirPath = await mkdtemp(join6(tmpdir(), tempDirPrefix)); + const tempDirPath = await mkdtemp(join7(tmpdir(), tempDirPrefix)); const urlPath = new URL(assetUrl).pathname; const fileName3 = urlPath.split("/").pop() || "asset"; - const downloadPath = join6(tempDirPath, fileName3); + const downloadPath = join7(tempDirPath, fileName3); const assetResponse = await fetchWithRetry(assetUrl, headers, "Failed to download asset"); if (!assetResponse.body) throw new Error("Response body is null"); const fileStream = createWriteStream2(downloadPath); @@ -136365,7 +136351,7 @@ async function installFromGithub(params) { log.debug(`\xBB downloaded asset to ${downloadPath}`); let cliPath; if (params.executablePath) { - cliPath = join6(tempDirPath, params.executablePath); + cliPath = join7(tempDirPath, params.executablePath); } else { cliPath = downloadPath; } @@ -136379,7 +136365,7 @@ async function installFromGithub(params) { async function installFromCurl(params) { log.info(`\xBB installing ${params.executableName}...`); const tempDir = process.env.PULLFROG_TEMP_DIR; - const installScriptPath = join6(tempDir, "install.sh"); + const installScriptPath = join7(tempDir, "install.sh"); log.debug(`\xBB downloading install script from ${params.installUrl}...`); const installScriptResponse = await fetch(params.installUrl); if (!installScriptResponse.ok) { @@ -136398,7 +136384,7 @@ async function installFromCurl(params) { // ensuring a fresh install for each run HOME: tempDir, // XDG_CONFIG_HOME must match HOME so CLI tools find config in the right place - XDG_CONFIG_HOME: join6(tempDir, ".config"), + XDG_CONFIG_HOME: join7(tempDir, ".config"), SHELL: process.env.SHELL, USER: process.env.USER }, @@ -136411,7 +136397,7 @@ async function installFromCurl(params) { `Failed to install ${params.executableName}. Install script exited with code ${installResult.status}. Output: ${errorOutput}` ); } - const cliPath = join6(tempDir, ".local", "bin", params.executableName); + const cliPath = join7(tempDir, ".local", "bin", params.executableName); if (!existsSync4(cliPath)) { throw new Error(`Executable not found at ${cliPath}`); } @@ -136583,8 +136569,8 @@ var messageHandlers = { }; // agents/codex.ts -import { mkdirSync as mkdirSync3, writeFileSync as writeFileSync2 } from "node:fs"; -import { join as join7 } from "node:path"; +import { mkdirSync as mkdirSync3, writeFileSync as writeFileSync3 } from "node:fs"; +import { join as join8 } from "node:path"; // node_modules/.pnpm/@openai+codex-sdk@0.80.0/node_modules/@openai/codex-sdk/dist/index.js import { promises as fs3 } from "fs"; @@ -136944,9 +136930,9 @@ var codexReasoningEffort = { max: "high" }; function writeCodexConfig(ctx) { - const codexDir = join7(ctx.tmpdir, ".codex"); + const codexDir = join8(ctx.tmpdir, ".codex"); mkdirSync3(codexDir, { recursive: true }); - const configPath = join7(codexDir, "config.toml"); + const configPath = join8(codexDir, "config.toml"); log.info(`\xBB adding MCP server '${ghPullfrogMcpName}' at ${ctx.mcpServerUrl}`); const mcpServerSections = [`[mcp_servers.${ghPullfrogMcpName}] url = "${ctx.mcpServerUrl}"`]; @@ -136958,7 +136944,7 @@ url = "${ctx.mcpServerUrl}"`]; } const featuresSection = features.length > 0 ? `[features] ${features.join("\n")}` : ""; - writeFileSync2( + writeFileSync3( configPath, `# written by pullfrog ${featuresSection} @@ -136981,7 +136967,7 @@ var codex = agent({ install: installCodex, run: async (ctx) => { const cliPath = await installCodex(); - const configDir = join7(ctx.tmpdir, ".config", "codex"); + const configDir = join8(ctx.tmpdir, ".config", "codex"); mkdirSync3(configDir, { recursive: true }); const codexDir = writeCodexConfig(ctx); process.env.HOME = ctx.tmpdir; @@ -137126,9 +137112,9 @@ var messageHandlers2 = { // agents/cursor.ts import { spawn as spawn5 } from "node:child_process"; -import { existsSync as existsSync5, mkdirSync as mkdirSync4, readFileSync as readFileSync3, writeFileSync as writeFileSync3 } from "node:fs"; +import { existsSync as existsSync5, mkdirSync as mkdirSync4, readFileSync as readFileSync3, writeFileSync as writeFileSync4 } from "node:fs"; import { homedir as homedir2 } from "node:os"; -import { join as join8 } from "node:path"; +import { join as join9 } from "node:path"; var cursorEffortModels = { mini: null, // use default (auto) @@ -137149,7 +137135,7 @@ var cursor = agent({ const cliPath = await installCursor(); configureCursorMcpServers(ctx); configureCursorTools(ctx); - const projectCliConfigPath = join8(process.cwd(), ".cursor", "cli.json"); + const projectCliConfigPath = join9(process.cwd(), ".cursor", "cli.json"); let modelOverride = null; if (existsSync5(projectCliConfigPath)) { try { @@ -137312,19 +137298,19 @@ var cursor = agent({ }); function configureCursorMcpServers(ctx) { const realHome = homedir2(); - const cursorConfigDir = join8(realHome, ".cursor"); - const mcpConfigPath = join8(cursorConfigDir, "mcp.json"); + const cursorConfigDir = join9(realHome, ".cursor"); + const mcpConfigPath = join9(cursorConfigDir, "mcp.json"); mkdirSync4(cursorConfigDir, { recursive: true }); const mcpServers = { [ghPullfrogMcpName]: { type: "http", url: ctx.mcpServerUrl } }; - writeFileSync3(mcpConfigPath, JSON.stringify({ mcpServers }, null, 2), "utf-8"); + writeFileSync4(mcpConfigPath, JSON.stringify({ mcpServers }, null, 2), "utf-8"); log.info(`\xBB MCP config written to ${mcpConfigPath}`); } function configureCursorTools(ctx) { const realHome = homedir2(); - const cursorConfigDir = join8(realHome, ".config", "cursor"); - const cliConfigPath = join8(cursorConfigDir, "cli-config.json"); + const cursorConfigDir = join9(realHome, ".config", "cursor"); + const cliConfigPath = join9(cursorConfigDir, "cli-config.json"); mkdirSync4(cursorConfigDir, { recursive: true }); const bash = ctx.payload.bash; const deny = []; @@ -137343,14 +137329,14 @@ function configureCursorTools(ctx) { networkAccess: "allowlist" }; } - writeFileSync3(cliConfigPath, JSON.stringify(config4, null, 2), "utf-8"); + writeFileSync4(cliConfigPath, JSON.stringify(config4, null, 2), "utf-8"); log.info(`\xBB CLI config written to ${cliConfigPath}`, JSON.stringify(config4, null, 2)); } // agents/gemini.ts -import { mkdirSync as mkdirSync5, readFileSync as readFileSync4, writeFileSync as writeFileSync4 } from "node:fs"; +import { mkdirSync as mkdirSync5, readFileSync as readFileSync4, writeFileSync as writeFileSync5 } from "node:fs"; import { homedir as homedir3 } from "node:os"; -import { join as join9 } from "node:path"; +import { join as join10 } from "node:path"; var geminiEffortConfig = { // https://ai.google.dev/gemini-api/docs/models // the docs mention needing to enable preview features for these models but if you @@ -137521,8 +137507,8 @@ function configureGeminiSettings(ctx) { const { model, thinkingLevel } = geminiEffortConfig[ctx.payload.effort]; log.info(`\xBB using model: ${model}, thinkingLevel: ${thinkingLevel}`); const realHome = homedir3(); - const geminiConfigDir = join9(realHome, ".gemini"); - const settingsPath = join9(geminiConfigDir, "settings.json"); + const geminiConfigDir = join10(realHome, ".gemini"); + const settingsPath = join10(geminiConfigDir, "settings.json"); mkdirSync5(geminiConfigDir, { recursive: true }); let existingSettings = {}; try { @@ -137559,7 +137545,7 @@ function configureGeminiSettings(ctx) { // v0.3.0+ nested format ...exclude.length > 0 && { tools: { exclude } } }; - writeFileSync4(settingsPath, JSON.stringify(newSettings, null, 2), "utf-8"); + writeFileSync5(settingsPath, JSON.stringify(newSettings, null, 2), "utf-8"); log.info(`\xBB Gemini settings written to ${settingsPath}`); if (exclude.length > 0) { log.info(`\xBB excluded tools: ${exclude.join(", ")}`); @@ -137568,8 +137554,8 @@ function configureGeminiSettings(ctx) { } // agents/opencode.ts -import { mkdirSync as mkdirSync6, writeFileSync as writeFileSync5 } from "node:fs"; -import { join as join10 } from "node:path"; +import { mkdirSync as mkdirSync6, writeFileSync as writeFileSync6 } from "node:fs"; +import { join as join11 } from "node:path"; async function installOpencode() { return await installFromNpmTarball({ packageName: "opencode-ai", @@ -137584,7 +137570,7 @@ var opencode = agent({ run: async (ctx) => { const cliPath = await installOpencode(); const tempHome = ctx.tmpdir; - const configDir = join10(tempHome, ".config", "opencode"); + const configDir = join11(tempHome, ".config", "opencode"); mkdirSync6(configDir, { recursive: true }); configureOpenCode(ctx); const args3 = ["run", ctx.instructions, "--format", "json"]; @@ -137592,7 +137578,7 @@ var opencode = agent({ const env3 = { ...process.env, HOME: tempHome, - XDG_CONFIG_HOME: join10(tempHome, ".config"), + XDG_CONFIG_HOME: join11(tempHome, ".config"), // set GOOGLE_GENERATIVE_AI_API_KEY alias for Google provider compatibility (if not already set) GOOGLE_GENERATIVE_AI_API_KEY: process.env.GOOGLE_GENERATIVE_AI_API_KEY || process.env.GEMINI_API_KEY }; @@ -137695,9 +137681,9 @@ var opencode = agent({ } }); function configureOpenCode(ctx) { - const configDir = join10(ctx.tmpdir, ".config", "opencode"); + const configDir = join11(ctx.tmpdir, ".config", "opencode"); mkdirSync6(configDir, { recursive: true }); - const configPath = join10(configDir, "opencode.json"); + const configPath = join11(configDir, "opencode.json"); const opencodeMcpServers = { [ghPullfrogMcpName]: { type: "remote", url: ctx.mcpServerUrl } }; @@ -137715,7 +137701,7 @@ function configureOpenCode(ctx) { }; const configJson = JSON.stringify(config4, null, 2); try { - writeFileSync5(configPath, configJson, "utf-8"); + writeFileSync6(configPath, configJson, "utf-8"); } catch (error50) { log.error( `failed to write OpenCode config to ${configPath}: ${error50 instanceof Error ? error50.message : String(error50)}` @@ -138388,9 +138374,9 @@ async function handleAgentResult(result) { import { execSync as execSync2 } from "node:child_process"; import { mkdtemp as mkdtemp2 } from "node:fs/promises"; import { tmpdir as tmpdir2 } from "node:os"; -import { join as join11 } from "node:path"; +import { join as join12 } from "node:path"; async function createTempDirectory() { - const sharedTempDir = await mkdtemp2(join11(tmpdir2(), "pullfrog-")); + const sharedTempDir = await mkdtemp2(join12(tmpdir2(), "pullfrog-")); process.env.PULLFROG_TEMP_DIR = sharedTempDir; log.info(`\xBB created temp dir at ${sharedTempDir}`); return sharedTempDir; diff --git a/external.ts b/external.ts index a9d8bd8..3e15cfd 100644 --- a/external.ts +++ b/external.ts @@ -201,30 +201,13 @@ interface WorkflowDispatchEvent extends BasePayloadEvent { trigger: "workflow_dispatch"; } -/** simplified review comment data for payload */ -export interface ReviewCommentData { - id: number; - body: string; - path: string; - line: number | null; - user: string | null; - html_url: string; - in_reply_to_id: number | null; -} - interface FixReviewEvent extends BasePayloadEvent { trigger: "fix_review"; issue_number: number; is_pr: true; review_id: number; - /** username of the person who triggered this action */ + /** username of the person who triggered this action - use with get_review_comments approved_by */ triggerer: string; - /** "all" to fix all comments, or specific comment IDs to fix */ - comment_ids: number[] | "all"; - /** comments the triggerer approved (via thumbs up) - these should be addressed */ - approved_comments: ReviewCommentData[]; - /** other comments in the review - for context only, do not address unless asked */ - unapproved_comments: ReviewCommentData[]; } interface ImplementPlanEvent extends BasePayloadEvent { diff --git a/mcp/review.ts b/mcp/review.ts index 84cac71..23e43e4 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -1,9 +1,9 @@ import type { RestEndpointMethodTypes } from "@octokit/rest"; import { type } from "arktype"; -import type { ToolContext } from "./server.ts"; import { buildPullfrogFooter } from "../utils/buildPullfrogFooter.ts"; import { log } from "../utils/cli.ts"; import { deleteProgressComment } from "./comment.ts"; +import type { ToolContext } from "./server.ts"; import { execute, tool } from "./shared.ts"; // one-shot review tool @@ -20,7 +20,7 @@ export const CreatePullRequestReview = type({ comments: type({ path: type.string.describe("The file path to comment on (relative to repo root)"), line: type.number.describe( - "Line number from the diff. Each code line shows 'OLD | NEW | TYPE | CODE'. Use the NEW column (second column)." + "End line of the comment range (or the only line if no start_line). From diff format 'OLD | NEW | TYPE | CODE', use the NEW column." ), side: type .enumerated("LEFT", "RIGHT") @@ -28,11 +28,18 @@ export const CreatePullRequestReview = type({ "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. When providing code suggestions, use GitHub's suggestion format with ```suggestion blocks to enable one-click apply. Only include explanatory text if the suggested code requires clarification." - ), + body: type.string + .describe("Explanatory comment text (optional if suggestion is provided)") + .optional(), + suggestion: type.string + .describe( + "Full replacement code for the line range. Replaces the commented lines entirely - can be more, fewer, or same number of lines." + ) + .optional(), start_line: type.number - .describe("Start line for multi-line comments (optional, for commenting on ranges)") + .describe( + "Start line for multi-line comments/suggestions. The range [start_line, line] defines which lines get replaced." + ) .optional(), }) .array() @@ -49,7 +56,7 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { "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. " + - "When suggesting code changes in comments, use GitHub's suggestion format (```suggestion blocks) to enable one-click apply.", + "Use 'suggestion' field to suggest a FULL REPLACEMENT of the associated line range. This will show up for users as a one-click apply suggestion. It doesn't need to be the same length as the original range, but the suggested code must work when applied.", parameters: CreatePullRequestReview, execute: execute(async ({ pull_number, body, commit_id, comments = [] }) => { // set PR context @@ -78,8 +85,17 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { type ReviewComment = (typeof params.comments & {})[number]; // convert comments to the format expected by GitHub API params.comments = comments.map((comment) => { + // build comment body with suggestion block if provided + let commentBody = comment.body || ""; + if (comment.suggestion !== undefined) { + const suggestionBlock = "```suggestion\n" + comment.suggestion + "\n```"; + commentBody = commentBody ? commentBody + "\n\n" + suggestionBlock : suggestionBlock; + } + const reviewComment: ReviewComment = { - ...comment, + path: comment.path, + line: comment.line, + body: commentBody, }; reviewComment.side = comment.side || "RIGHT"; if (comment.start_line) { @@ -102,7 +118,12 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { const fixApprovedUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix-approved&review_id=${reviewId}`; const footer = buildPullfrogFooter({ - workflowRun: { owner: ctx.repo.owner, repo: ctx.repo.name, runId: ctx.runId, jobId: ctx.jobId }, + workflowRun: { + owner: ctx.repo.owner, + repo: ctx.repo.name, + runId: ctx.runId, + jobId: ctx.jobId, + }, customParts: [`[Fix all ➔](${fixAllUrl})`, `[Fix 👍s ➔](${fixApprovedUrl})`], }); diff --git a/mcp/reviewComments.ts b/mcp/reviewComments.ts index 557acc9..7e0eaa2 100644 --- a/mcp/reviewComments.ts +++ b/mcp/reviewComments.ts @@ -1,187 +1,108 @@ +import { writeFileSync } from "node:fs"; +import { join } from "node:path"; import { type } from "arktype"; +import { log } from "../utils/log.ts"; import type { ToolContext } from "./server.ts"; import { execute, tool } from "./shared.ts"; -// graphql query to fetch all review threads with comments and replies -// note: diffSide and startDiffSide are on the thread, not the comment -const REVIEW_THREADS_QUERY = ` -query ($owner: String!, $repo: String!, $pullNumber: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $pullNumber) { - reviewThreads(first: 100) { - nodes { - diffSide - startDiffSide - comments(first: 100) { - nodes { - id - databaseId - body - path - line - startLine - url - author { - login - } - createdAt - updatedAt - pullRequestReview { - databaseId - } - replyTo { - databaseId - } - } - } - } - } - } - } -} -`; - -// graphql response types (nodes arrays can contain nulls per GitHub GraphQL spec) -type GraphQLReviewComment = { - id: string; - databaseId: number; - body: string; - path: string; - line: number | null; - startLine: number | null; - url: string; - author: { - login: string; - } | null; - createdAt: string; - updatedAt: string; - pullRequestReview: { - databaseId: number; - } | null; - replyTo: { - databaseId: number; - } | null; -}; - -type GraphQLReviewThread = { - diffSide: "LEFT" | "RIGHT"; - startDiffSide: "LEFT" | "RIGHT" | null; - comments: { - nodes: (GraphQLReviewComment | null)[] | null; - } | null; -} | null; - -type GraphQLResponse = { - repository: { - pullRequest: { - reviewThreads: { - nodes: (GraphQLReviewThread | null)[] | null; - } | null; - } | null; - } | null; -}; - 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") + .optional(), }); export function GetReviewCommentsTool(ctx: ToolContext) { return tool({ name: "get_review_comments", description: - "Get all review comments and their replies for a specific pull request review. Returns line-by-line comments that were left on specific code locations, including any threaded replies.", + "Get review comments for a pull request review, including diff context. " + + "When approved_by is provided, only returns comments that user approved with 👍. " + + "Returns commentsPath pointing to a file with full comment details.", parameters: GetReviewComments, - execute: execute(async ({ pull_number, review_id }) => { - // fetch all review threads using graphql - const response = await ctx.octokit.graphql(REVIEW_THREADS_QUERY, { - owner: ctx.repo.owner, - repo: ctx.repo.name, - pullNumber: pull_number, - }); + execute: execute(async ({ pull_number, review_id, approved_by }) => { + // fetch all review comments via REST API (includes diff_hunk) + const allComments = await ctx.octokit.paginate(ctx.octokit.rest.pulls.listReviewComments, { + owner: ctx.repo.owner, + repo: ctx.repo.name, + pull_number, + }); - const pullRequest = response.repository?.pullRequest; - if (!pullRequest) { - return { - review_id, - pull_number, - comments: [], - count: 0, - }; - } + // filter to target review + let reviewComments = allComments.filter((c) => c.pull_request_review_id === review_id); - const threadNodes = pullRequest.reviewThreads?.nodes; - if (!threadNodes) { - return { - review_id, - pull_number, - comments: [], - count: 0, - }; - } - - const allComments: { - id: number; - body: string; - path: string; - line: number | null; - side: "LEFT" | "RIGHT"; - start_line: number | null; - start_side: "LEFT" | "RIGHT" | null; - user: string | null; - created_at: string; - updated_at: string; - html_url: string; - in_reply_to_id: number | null; - pull_request_review_id: number | null; - }[] = []; - - // iterate through all threads (filter out nulls) - for (const thread of threadNodes) { - if (!thread?.comments?.nodes) continue; - - // filter out null comments - const threadComments = thread.comments.nodes.filter( - (c): c is GraphQLReviewComment => c !== null + // filter by thumbs up if approved_by is specified + if (approved_by) { + const approvedIds = new Set(); + for (const comment of reviewComments) { + const reactions = await ctx.octokit.rest.reactions.listForPullRequestReviewComment({ + owner: ctx.repo.owner, + repo: ctx.repo.name, + comment_id: comment.id, + }); + const hasThumbsUp = reactions.data.some( + (r) => r.content === "+1" && r.user?.login === approved_by ); - if (threadComments.length === 0) continue; - - // find the root comment (the one with replyTo == null) to determine thread ownership - const rootComment = threadComments.find((c) => c.replyTo === null); - if (!rootComment) continue; - - // check if this thread belongs to the target review using the root comment - const threadBelongsToReview = rootComment.pullRequestReview?.databaseId === review_id; - if (!threadBelongsToReview) continue; - - // include all comments from this thread (original + replies) - // side info comes from thread level, not comment level - for (const comment of threadComments) { - allComments.push({ - id: comment.databaseId, - body: comment.body, - path: comment.path, - line: comment.line, - start_line: comment.startLine, - side: thread.diffSide, - start_side: thread.startDiffSide, - user: comment.author?.login ?? null, - created_at: comment.createdAt, - updated_at: comment.updatedAt, - html_url: comment.url, - in_reply_to_id: comment.replyTo?.databaseId ?? null, - pull_request_review_id: comment.pullRequestReview?.databaseId ?? null, - }); - } + if (hasThumbsUp) approvedIds.add(comment.id); } + reviewComments = reviewComments.filter((c) => approvedIds.has(c.id)); + } + if (reviewComments.length === 0) { return { review_id, pull_number, - comments: allComments, - count: allComments.length, + count: 0, + commentsPath: null, + message: approved_by + ? `No comments with 👍 from ${approved_by}` + : "No comments found for this review", }; - }), + } + + // format comments with diff context + const lines: string[] = []; + for (const comment of reviewComments) { + lines.push(`${"=".repeat(60)}`); + lines.push(`COMMENT #${comment.id} by @${comment.user?.login ?? "unknown"}`); + lines.push(`File: ${comment.path}:${comment.line ?? comment.original_line ?? "?"}`); + if (comment.in_reply_to_id) { + lines.push(`Reply to: #${comment.in_reply_to_id}`); + } + lines.push(""); + if (comment.diff_hunk) { + lines.push("```diff"); + lines.push(comment.diff_hunk); + lines.push("```"); + lines.push(""); + } + lines.push("Comment:"); + lines.push(comment.body); + lines.push(""); + } + + const content = lines.join("\n"); + + // 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}.txt` + : `review-${review_id}-comments.txt`; + const commentsPath = join(tempDir, filename); + writeFileSync(commentsPath, content); + log.debug(`wrote ${reviewComments.length} comments to ${commentsPath}`); + + return { + review_id, + pull_number, + count: reviewComments.length, + commentsPath, + }; + }), }); } @@ -209,9 +130,7 @@ export function ListPullRequestReviewsTool(ctx: ToolContext) { body: review.body, state: review.state, user: review.user?.login, - commit_id: review.commit_id, submitted_at: review.submitted_at, - html_url: review.html_url, })), count: reviews.length, };