From 956245962e36de8e68f4d50e31e8dee0e4d6425f Mon Sep 17 00:00:00 2001 From: Colin McDonnell Date: Tue, 16 Dec 2025 19:56:09 -0800 Subject: [PATCH] Improve reviews --- main.ts | 5 + mcp/review.ts | 265 ++++++++++++++++++++++++++++++++++++++++++++++++++ mcp/server.ts | 10 +- modes.ts | 40 ++++---- 4 files changed, 302 insertions(+), 18 deletions(-) diff --git a/main.ts b/main.ts index df25ebb..0d0e856 100644 --- a/main.ts +++ b/main.ts @@ -11,6 +11,7 @@ import type { AgentName, Payload } from "./external.ts"; import { agentsManifest } from "./external.ts"; import { ensureProgressCommentUpdated, reportProgress } from "./mcp/comment.ts"; import { createMcpConfigs } from "./mcp/config.ts"; +import type { ReviewState } from "./mcp/review.ts"; import { startMcpHttpServer } from "./mcp/server.ts"; import { getModes, type Mode, modes } from "./modes.ts"; import packageJson from "./package.json" with { type: "json" }; @@ -242,6 +243,9 @@ export interface Context { // workflow run info runId: string; jobId: string | undefined; + + // iterative review state (set by start_review, cleared by submit_review) + reviewState: ReviewState | undefined; } async function initializeContext( @@ -259,6 +263,7 @@ async function initializeContext( | "prepResults" | "runId" | "jobId" + | "reviewState" > > { log.info(`🐸 Running pullfrog/action@${packageJson.version}...`); diff --git a/mcp/review.ts b/mcp/review.ts index a9707ce..fa2fb00 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -1,3 +1,6 @@ +import { randomBytes } from "node:crypto"; +import { writeFileSync } from "node:fs"; +import { join } from "node:path"; import type { RestEndpointMethodTypes } from "@octokit/rest"; import { type } from "arktype"; import type { Context } from "../main.ts"; @@ -5,6 +8,267 @@ import { buildPullfrogFooter } from "../utils/buildPullfrogFooter.ts"; import { deleteProgressComment } from "./comment.ts"; import { execute, tool } from "./shared.ts"; +// graphql mutation to create a pending review +const ADD_PULL_REQUEST_REVIEW = ` +mutation AddPullRequestReview($pullRequestId: ID!) { + addPullRequestReview(input: { pullRequestId: $pullRequestId, event: PENDING }) { + pullRequestReview { + id + databaseId + } + } +} +`; + +// graphql mutation to add a comment thread to a pending review +const ADD_PULL_REQUEST_REVIEW_THREAD = ` +mutation AddPullRequestReviewThread($pullRequestReviewId: ID!, $path: String!, $line: Int!, $body: String!, $side: DiffSide) { + addPullRequestReviewThread(input: { + pullRequestReviewId: $pullRequestReviewId, + path: $path, + line: $line, + body: $body, + side: $side + }) { + thread { + id + } + } +} +`; + +// graphql mutation to submit a pending review +const SUBMIT_PULL_REQUEST_REVIEW = ` +mutation SubmitPullRequestReview($pullRequestReviewId: ID!, $body: String, $event: PullRequestReviewEvent!) { + submitPullRequestReview(input: { + pullRequestReviewId: $pullRequestReviewId, + body: $body, + event: $event + }) { + pullRequestReview { + id + databaseId + state + url + } + } +} +`; + +// graphql response types +type AddPullRequestReviewResponse = { + addPullRequestReview: { + pullRequestReview: { + id: string; + databaseId: number; + }; + }; +}; + +type AddPullRequestReviewThreadResponse = { + addPullRequestReviewThread: { + thread: { + id: string; + }; + }; +}; + +type SubmitPullRequestReviewResponse = { + submitPullRequestReview: { + pullRequestReview: { + id: string; + databaseId: number; + state: string; + url: string; + }; + }; +}; + +// review state stored in ctx +export interface ReviewState { + reviewId: string; // graphql node ID + reviewDatabaseId: number; // rest API ID + pullNumber: number; + scratchpadPath: string; + commentCount: number; +} + +// start_review tool +export const StartReview = type({ + pull_number: type.number.describe("The pull request number to review"), +}); + +export function StartReviewTool(ctx: Context) { + return tool({ + name: "start_review", + description: + "Start a new review session for a pull request. Creates a scratchpad file for gathering thoughts and a pending review on GitHub. Must be called before add_review_comment.", + parameters: StartReview, + execute: execute(ctx, async ({ pull_number }) => { + // check if review already started + if (ctx.reviewState) { + throw new Error( + `Review session already started for PR #${ctx.reviewState.pullNumber}. Call submit_review first to finish it.` + ); + } + + // get the PR to get its node_id for GraphQL + const pr = await ctx.octokit.rest.pulls.get({ + owner: ctx.owner, + repo: ctx.name, + pull_number, + }); + + // create pending review via GraphQL + const response = await ctx.octokit.graphql( + ADD_PULL_REQUEST_REVIEW, + { + pullRequestId: pr.data.node_id, + } + ); + + const reviewId = response.addPullRequestReview.pullRequestReview.id; + const reviewDatabaseId = response.addPullRequestReview.pullRequestReview.databaseId; + + // create scratchpad file + const scratchpadId = randomBytes(4).toString("hex"); + const scratchpadPath = join(ctx.sharedTempDir, `pullfrog-review-${scratchpadId}.md`); + const scratchpadContent = `# Review ${scratchpadId}\n\n`; + writeFileSync(scratchpadPath, scratchpadContent); + + // store review state in ctx + ctx.reviewState = { + reviewId, + reviewDatabaseId, + pullNumber: pull_number, + scratchpadPath, + commentCount: 0, + }; + + return { + reviewId: scratchpadId, + scratchpadPath, + message: `Review session started. Use the scratchpad file to gather your thoughts, then call add_review_comment for each comment.`, + }; + }), + }); +} + +// add_review_comment tool +export const AddReviewComment = 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 - the NEW file line number)" + ), + body: type.string.describe("The comment text for this specific line"), + side: type + .enumerated("LEFT", "RIGHT") + .describe("Side of the diff: LEFT (old code) or RIGHT (new code). Defaults to RIGHT.") + .optional(), +}); + +export function AddReviewCommentTool(ctx: Context) { + return tool({ + name: "add_review_comment", + description: + "Add a comment to the current review session. Must call start_review first. Comments are stored in draft state until submit_review is called.", + parameters: AddReviewComment, + execute: execute(ctx, async ({ path, line, body, side }) => { + // check if review started + if (!ctx.reviewState) { + throw new Error("No review session started. Call start_review first."); + } + + // add comment thread via GraphQL + await ctx.octokit.graphql( + ADD_PULL_REQUEST_REVIEW_THREAD, + { + pullRequestReviewId: ctx.reviewState.reviewId, + path, + line, + body, + side: side || "RIGHT", + } + ); + + ctx.reviewState.commentCount++; + + return { + success: true, + commentCount: ctx.reviewState.commentCount, + message: `Comment added to ${path}:${line}`, + }; + }), + }); +} + +// submit_review tool +export const SubmitReview = type({ + body: type.string + .describe( + "Review body text. Typically 1-3 sentences with high-level overview and urgency level. Action links are auto-appended." + ) + .optional(), +}); + +export function SubmitReviewTool(ctx: Context) { + return tool({ + name: "submit_review", + description: + "Submit the current review session. All comments added via add_review_comment will be published. Must call start_review first.", + parameters: SubmitReview, + execute: execute(ctx, async ({ body }) => { + // check if review started + if (!ctx.reviewState) { + throw new Error("No review session started. Call start_review first."); + } + + const pullNumber = ctx.reviewState.pullNumber; + const reviewDatabaseId = ctx.reviewState.reviewDatabaseId; + + // build quick links footer + const apiUrl = process.env.API_URL || "https://pullfrog.com"; + const fixAllUrl = `${apiUrl}/trigger/${ctx.owner}/${ctx.name}/${pullNumber}?action=fix&review_id=${reviewDatabaseId}`; + const fixApprovedUrl = `${apiUrl}/trigger/${ctx.owner}/${ctx.name}/${pullNumber}?action=fix-approved&review_id=${reviewDatabaseId}`; + + const footer = buildPullfrogFooter({ + workflowRun: { owner: ctx.owner, repo: ctx.name, runId: ctx.runId, jobId: ctx.jobId }, + customParts: [`[Fix all ➔](${fixAllUrl})`, `[Fix 👍s ➔](${fixApprovedUrl})`], + }); + + const bodyWithFooter = (body || "") + footer; + + // submit the review via GraphQL + const response = await ctx.octokit.graphql( + SUBMIT_PULL_REQUEST_REVIEW, + { + pullRequestReviewId: ctx.reviewState.reviewId, + body: bodyWithFooter, + event: "COMMENT", + } + ); + + const result = response.submitPullRequestReview.pullRequestReview; + const commentCount = ctx.reviewState.commentCount; + + // clear review state + ctx.reviewState = undefined; + + // delete progress comment + await deleteProgressComment(ctx); + + return { + success: true, + reviewId: result.databaseId, + html_url: result.url, + state: result.state, + commentCount, + }; + }), + }); +} + +// legacy tool - kept for backwards compatibility export const Review = type({ pull_number: type.number.describe("The pull request number to review"), body: type.string @@ -46,6 +310,7 @@ export function ReviewTool(ctx: Context) { return tool({ name: "submit_pull_request_review", description: + "DEPRECATED: Use start_review, add_review_comment, and submit_review instead for iterative review workflow. " + "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.", diff --git a/mcp/server.ts b/mcp/server.ts index 593274f..f823177 100644 --- a/mcp/server.ts +++ b/mcp/server.ts @@ -21,7 +21,12 @@ import { IssueInfoTool } from "./issueInfo.ts"; import { AddLabelsTool } from "./labels.ts"; import { PullRequestTool } from "./pr.ts"; import { PullRequestInfoTool } from "./prInfo.ts"; -import { ReviewTool } from "./review.ts"; +import { + AddReviewCommentTool, + ReviewTool, + StartReviewTool, + SubmitReviewTool, +} from "./review.ts"; import { GetReviewCommentsTool, ListPullRequestReviewsTool } from "./reviewComments.ts"; import { SelectModeTool } from "./selectMode.ts"; import { addTools, isProgressCommentDisabled } from "./shared.ts"; @@ -78,6 +83,9 @@ export async function startMcpHttpServer( GetIssueEventsTool(ctx), PullRequestTool(ctx), ReviewTool(ctx), + StartReviewTool(ctx), + AddReviewCommentTool(ctx), + SubmitReviewTool(ctx), PullRequestInfoTool(ctx), CheckoutPrTool(ctx), GetReviewCommentsTool(ctx), diff --git a/modes.ts b/modes.ts index 65f80b5..f4d07e0 100644 --- a/modes.ts +++ b/modes.ts @@ -95,30 +95,36 @@ ${ prompt: `Follow these steps: 1. Checkout the PR using ${ghPullfrogMcpName}/checkout_pr with the PR number. This fetches the PR branch and base branch, preparing the repo for review. -2. **IMPORTANT**: After calling checkout_pr, the PR branch is checked out locally. View diff using: \`git diff origin/..HEAD\` (replace with 'base' from checkout_pr result, e.g., \`git diff origin/main..HEAD\`). Use two dots (..) not three dots (...) for reliable diffs. Do NOT use \`origin/\` - the branch is checked out locally, not as a remote tracking branch. This works for both same-repo and fork PRs. +2. **IMPORTANT**: After calling ${ghPullfrogMcpName}/checkout_pr, the PR branch is checked out locally. View diff using: \`git diff origin/..HEAD\` (replace with 'base' from checkout_pr result, e.g., \`git diff origin/main..HEAD\`). Use two dots (..) not three dots (...) for reliable diffs. Do NOT use \`origin/\` - the branch is checked out locally, not as a remote tracking branch. This works for both same-repo and fork PRs. -3. Read files from the checked-out PR branch to understand the implementation. Always use **relative paths** from repo root (e.g., \`src/index.ts\`), never absolute paths. +3. Start review session using ${ghPullfrogMcpName}/start_review. This creates a scratchpad file at a temp path (e.g., \`/tmp/pullfrog-review-abc123.md\`) and returns a session ID. The scratchpad file header contains the session ID for reference. Use this file as free-form space to gather your thoughts before adding comments. -4. Submit review using ${ghPullfrogMcpName}/submit_pull_request_review +4. **ANALYZE** - Use the scratchpad to gather your thoughts: + - Summarize what changes this PR makes + - Evaluate the approach - is it sound? If not, **stop here** and leave feedback on the approach. Don't waste time on implementation details if the approach is wrong. + - If approach is sound, analyze implementation - consider potential issues per file + - Identify bugs, security issues, edge cases + +5. **SELF-CRITIQUE** - Before adding comments, review your scratchpad: + - Remove nitpicks unless explicitly requested. Think documentation, JSDoc/docstrings, useless comments (compliments) + - Your level of nitpickiness should be proportional to the current state of the codebase. Try to guess how much the user will care about a specific critique. + +6. Add inline review comments one0-by-one using ${ghPullfrogMcpName}/add_review_comment + - Use **relative paths** from repo root (e.g., \`packages/core/src/utils.ts\`) + - Use the NEW file line number from the diff (shown after \`+\` in hunk headers like \`@@ -10,5 +12,8 @@\` means new file starts at line 12) + - Only comment on lines that appear in the diff. GitHub will reject comments on unchanged lines. + - For issues appearing in multiple places, comment on the FIRST occurrence and reference others (e.g., "also at lines X, Y") + +7. Submit the review using ${ghPullfrogMcpName}/submit_review + - The "body" field is ONLY for: (1) a 1-3 sentence high-level overview, (2) urgency level (e.g., "minor suggestions" vs "blocking issues"), (3) critical security callouts (e.g., API key exposure) **GENERAL GUIDANCE** - Do not leave any comments that are not potentially actionable. Do not leave complimentary comments just to be nice. -- *CRITICAL* — Use **relative paths** from repo root (e.g., \`packages/core/src/utils.ts\`) - - For line numbers, use the NEW file line number from the diff (shown after \`+\` in hunk headers like \`@@ -10,5 +12,8 @@\` means new file starts at line 12) - - Only comment on lines that appear in the diff. GitHub will reject comments on unchanged lines. +- Do not nitpick unless instructed explicitly to do so by the user's additional instructions. This includes: requesting documentation/docstrings/JSDoc. - **CRITICAL: Prioritize per-line feedback over summary text.** - - ALL specific feedback MUST go in the 'comments' array with file paths and line numbers from the diff - - For issues appearing in multiple places, comment on the FIRST occurrence and reference others (e.g., "also at lines X, Y" or "similar issue in otherFile.ts:42") - - The "body" field is ONLY for: (1) a 1-2 sentence high-level overview, (2) urgency level (e.g., "minor suggestions" vs "blocking issues"), (3) critical security callouts (e.g., API key exposure) - - 95%+ of review content should be in per-line comments; the body should be just a couple sentences - - The review body will include quick action links for addressing feedback, so keep it concise -- Do not nitpick unless instructed explicity to do so by the user's additional instructions. This includes: requesting documentation/docstrings/JSDoc. -- The review should be thoughtful. When evaluating complex changes, consider the following conceptual approach: - - 1) conceptualize the changes made. make sure you understand it. - - 2) evaluate conceptual approach. leave feedback as needed. - - 3) if the conceptual approach looks sound, evaluate the implementation. leave feedback as needed. consider everything, but especially edge cases, security, correctness, and performance. leave feedback as needed. - - 4) only leave nitpick/housekeeping comments if instructed explicity to do so by the user's additional instructions. + - All specific feedback MUST go in inline review comments with file paths and line numbers from the diff + - The vast majority of review content should be in inline review comments; the body should be brief and only summarize the urgency of the review and any cross-cutting concerns. `, }, {