diff --git a/entry b/entry index e052997..1a13d59 100755 --- a/entry +++ b/entry @@ -83328,7 +83328,7 @@ function query({ // package.json var package_default = { name: "@pullfrog/action", - version: "0.0.152", + version: "0.0.153", type: "module", files: [ "index.js", @@ -92628,36 +92628,38 @@ ${disableProgressComment ? "" : ` name: "Review", description: "Review code, PRs, or implementations; provide feedback or suggestions; identify issues; or check code quality, style, and correctness", prompt: `Follow these steps: -1. Checkout the PR using ${ghPullfrogMcpName}/checkout_pr with the PR number. This fetches the PR branch and returns the PR diff in the \`diff\` field of the response. Use this diff for your review - it shows exactly what's in the PR (fetched via GitHub API, so it's not affected by main advancing after the branch was created). -2. Start review session using ${ghPullfrogMcpName}/start_review. This creates a pending review on GitHub and returns analysis guidance. Follow the guidance before adding comments. +1. **CHECKOUT** - Use ${ghPullfrogMcpName}/checkout_pr with the PR number. This fetches the PR branch and returns the diff. Use this diff for your review - it shows exactly what's in the PR. -3. **ANALYZE** - Before adding any comments, think through: +2. **UNDERSTAND CONTEXT** - Read the modified files to understand the changes in context. Don't just look at the diff - understand how the changes affect the overall codebase. + +3. **ANALYZE** - Think through: - What does this PR change? Summarize in 1-2 sentences. - - Is the approach sound? If not, **stop here** and comment on the approach first. Don't waste time on implementation details if the approach is wrong. + - 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. **BEFORE COMMENTING** - For each potential comment, ask yourself: - - Is this a nitpick? Skip it unless explicitly requested. - - Would the codebase maintainer care about this feedback, based on what you can infer about the code quality standards in this repo? +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 -5. Add inline review comments one-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") +5. **SELF-CRITIQUE** - Before submitting, review your draft: + - Remove nitpicks unless the user explicitly told you to be nitpicky. Nitpicks may include: requesting documentation/docstrings/JSDoc, commenting on minor code/whitespace formatting, commenting on small changes unrelated to the main changes. + - Ensure each comment is actionable - would the author know exactly what to do? + - Would the codebase maintainer care about this feedback? + - If you have approach-level concerns, consider whether implementation-level comments are worth including + - For issues appearing in multiple places, keep only the FIRST occurrence and reference others (e.g., "also at lines X, Y") -6. 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) +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) -**GENERAL GUIDANCE** - -- Do not leave any comments that are not potentially actionable. Do not leave complimentary comments just to be nice. -- 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 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. - ` +**CRITICAL RULES** +- 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 +` }, { name: "Plan", @@ -123106,7 +123108,13 @@ function CheckoutPrTool(ctx) { mediaType: { format: "diff" } }); const diffContent = diffResponse.data; - const diffPath = join8(process.env.PULLFROG_TEMP_DIR, `pr-${pull_number}.diff`); + const tempDir = process.env.PULLFROG_TEMP_DIR; + if (!tempDir) { + throw new Error( + "PULLFROG_TEMP_DIR not set - checkout_pr must run in pullfrog action context" + ); + } + const diffPath = join8(tempDir, `pr-${pull_number}.diff`); writeFileSync4(diffPath, diffContent); log.debug(`wrote diff to ${diffPath} (${diffContent.length} bytes)`); const MAX_INLINE_DIFF_SIZE = 50 * 1024; @@ -124466,224 +124474,7 @@ function PullRequestInfoTool(ctx) { } // mcp/review.ts -var 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 - } - } -} -`; -async function findPendingReview(ctx, pull_number) { - const reviews = await ctx.octokit.rest.pulls.listReviews({ - owner: ctx.owner, - repo: ctx.name, - pull_number, - per_page: 100 - }); - const pendingReview = reviews.data.find((r) => r.state === "PENDING"); - if (pendingReview) { - return { id: pendingReview.id, node_id: pendingReview.node_id }; - } - return null; -} -var StartReview = type({ - pull_number: type.number.describe("The pull request number to review") -}); -function StartReviewTool(ctx) { - return tool({ - name: "start_review", - description: "Start a new review session for a pull request. Creates a pending review on GitHub. Must be called before add_review_comment.", - parameters: StartReview, - execute: execute(async ({ pull_number }) => { - if (ctx.toolState.review) { - throw new Error( - `Review session already in progress. Call submit_review first to finish it.` - ); - } - const pr = await ctx.octokit.rest.pulls.get({ - owner: ctx.owner, - repo: ctx.name, - pull_number - }); - let reviewId; - let reviewNodeId; - log.debug(`creating pending review for PR #${pull_number}...`); - try { - const result = await ctx.octokit.rest.pulls.createReview({ - owner: ctx.owner, - repo: ctx.name, - pull_number, - commit_id: pr.data.head.sha - // no 'event' = PENDING review - }); - log.debug(`createReview response: ${JSON.stringify(result.data)}`); - if (!result.data.id || !result.data.node_id) { - log.debug(result); - throw new Error( - `createReview returned invalid data: id=${result.data.id}, node_id=${result.data.node_id}` - ); - } - reviewId = result.data.id; - reviewNodeId = result.data.node_id; - log.debug(`created new pending review: id=${reviewId}`); - } catch (error41) { - const errorMessage = error41 instanceof Error ? error41.message : String(error41); - log.debug(`createReview failed: ${errorMessage}`); - if (errorMessage.includes("pending review")) { - log.debug(`pending review already exists, fetching existing review...`); - const existing = await findPendingReview(ctx, pull_number); - if (!existing) { - throw new Error( - "GitHub says a pending review exists but we couldn't find it. Try again or check the PR reviews." - ); - } - reviewId = existing.id; - reviewNodeId = existing.node_id; - log.debug(`reusing existing pending review: id=${reviewId}`); - } else { - throw error41; - } - } - ctx.toolState.prNumber = pull_number; - ctx.toolState.review = { - nodeId: reviewNodeId, - id: reviewId - }; - log.debug(`review session started: id=${reviewId}, nodeId=${reviewNodeId}`); - return { - message: `Review session started for PR #${pull_number}. Add comments with add_review_comment, then submit with submit_review.` - }; - }) - }); -} -var 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() -}); -function AddReviewCommentTool(ctx) { - 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(async ({ path: path4, line, body, side }) => { - if (!ctx.toolState.review) { - throw new Error("No review session started. Call start_review first."); - } - const reviewNodeId = ctx.toolState.review.nodeId; - log.debug( - `adding review comment: reviewNodeId=${reviewNodeId}, path=${path4}, line=${line}, side=${side || "RIGHT"}` - ); - let result; - try { - result = await ctx.octokit.graphql( - ADD_PULL_REQUEST_REVIEW_THREAD, - { - pullRequestReviewId: reviewNodeId, - path: path4, - line, - body, - side: side || "RIGHT" - } - ); - log.debug(`addPullRequestReviewThread response: ${JSON.stringify(result)}`); - } catch (error41) { - const errorMsg = error41 instanceof Error ? error41.message : String(error41); - log.debug(`addPullRequestReviewThread error: ${errorMsg}`); - throw new Error( - `Failed to add comment to ${path4}:${line}. GraphQL error: ${errorMsg}. Ensure the line is part of the diff and the path is correct.` - ); - } - if (!result) { - throw new Error( - `Failed to add comment to ${path4}:${line}. GraphQL returned null response.` - ); - } - if (!result.addPullRequestReviewThread) { - throw new Error( - `Failed to add comment to ${path4}:${line}. addPullRequestReviewThread is null. Response: ${JSON.stringify(result)}` - ); - } - if (!result.addPullRequestReviewThread.thread) { - throw new Error( - `Failed to add comment to ${path4}:${line}. thread is null. The line must be part of the diff. Response: ${JSON.stringify(result)}` - ); - } - const threadId = result.addPullRequestReviewThread.thread.id; - log.debug(`review comment added: threadId=${threadId}`); - return { - success: true, - message: `Comment added to ${path4}:${line}`, - threadId - }; - }) - }); -} -var 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() -}); -function SubmitReviewTool(ctx) { - 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(async ({ body }) => { - if (!ctx.toolState.review) { - throw new Error("No review session started. Call start_review first."); - } - if (ctx.toolState.prNumber === void 0) { - throw new Error("No PR context. Call checkout_pr or start_review first."); - } - const reviewId = ctx.toolState.review.id; - log.debug( - `submitting review: id=${reviewId}, nodeId=${ctx.toolState.review.nodeId}, prNumber=${ctx.toolState.prNumber}` - ); - const apiUrl = process.env.API_URL || "https://pullfrog.com"; - const fixAllUrl = `${apiUrl}/trigger/${ctx.owner}/${ctx.name}/${ctx.toolState.prNumber}?action=fix&review_id=${reviewId}`; - const fixApprovedUrl = `${apiUrl}/trigger/${ctx.owner}/${ctx.name}/${ctx.toolState.prNumber}?action=fix-approved&review_id=${reviewId}`; - const footer = buildPullfrogFooter({ - workflowRun: { owner: ctx.owner, repo: ctx.name, runId: ctx.runId, jobId: ctx.jobId }, - customParts: [`[Fix all \u2794](${fixAllUrl})`, `[Fix \u{1F44D}s \u2794](${fixApprovedUrl})`] - }); - const bodyWithFooter = (body || "") + footer; - const result = await ctx.octokit.rest.pulls.submitReview({ - owner: ctx.owner, - repo: ctx.name, - pull_number: ctx.toolState.prNumber, - review_id: reviewId, - event: "COMMENT", - body: bodyWithFooter - }); - log.debug(`submitReview response: ${JSON.stringify(result.data)}`); - if (!result.data.id) { - throw new Error(`submitReview returned invalid data: ${JSON.stringify(result.data)}`); - } - log.debug(`review submitted: reviewId=${result.data.id}, state=${result.data.state}`); - delete ctx.toolState.review; - await deleteProgressComment(ctx); - return { - success: true, - reviewId: result.data.id, - html_url: result.data.html_url, - state: result.data.state - }; - }) - }); -} -var Review = type({ +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." @@ -124706,6 +124497,76 @@ var Review = type({ "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)." ).optional() }); +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.", + parameters: CreatePullRequestReview, + execute: execute(async ({ pull_number, body, commit_id, comments = [] }) => { + ctx.toolState.prNumber = pull_number; + const pr = await ctx.octokit.rest.pulls.get({ + owner: ctx.owner, + repo: ctx.name, + pull_number + }); + const params = { + owner: ctx.owner, + repo: ctx.name, + pull_number, + event: "COMMENT" + }; + if (body) params.body = body; + if (commit_id) { + params.commit_id = commit_id; + } else { + params.commit_id = pr.data.head.sha; + } + if (comments.length > 0) { + params.comments = comments.map((comment) => { + const reviewComment = { + ...comment + }; + reviewComment.side = comment.side || "RIGHT"; + if (comment.start_line) { + reviewComment.start_line = comment.start_line; + reviewComment.start_side = comment.side || "RIGHT"; + } + return reviewComment; + }); + } + const result = await ctx.octokit.rest.pulls.createReview(params); + log.debug(`createReview response: ${JSON.stringify(result.data)}`); + if (!result.data.id) { + throw new Error(`createReview returned invalid data: ${JSON.stringify(result.data)}`); + } + const reviewId = result.data.id; + const apiUrl = process.env.API_URL || "https://pullfrog.com"; + const fixAllUrl = `${apiUrl}/trigger/${ctx.owner}/${ctx.name}/${pull_number}?action=fix&review_id=${reviewId}`; + const fixApprovedUrl = `${apiUrl}/trigger/${ctx.owner}/${ctx.name}/${pull_number}?action=fix-approved&review_id=${reviewId}`; + const footer = buildPullfrogFooter({ + workflowRun: { owner: ctx.owner, repo: ctx.name, runId: ctx.runId, jobId: ctx.jobId }, + customParts: [`[Fix all \u2794](${fixAllUrl})`, `[Fix \u{1F44D}s \u2794](${fixApprovedUrl})`] + }); + const updatedBody = (body || "") + footer; + await ctx.octokit.rest.pulls.updateReview({ + owner: ctx.owner, + repo: ctx.name, + pull_number, + review_id: reviewId, + body: updatedBody + }); + await deleteProgressComment(ctx); + return { + success: true, + reviewId, + html_url: result.data.html_url, + state: result.data.state, + user: result.data.user?.login, + submitted_at: result.data.submitted_at + }; + }) + }); +} // mcp/reviewComments.ts var REVIEW_THREADS_QUERY = ` @@ -124917,10 +124778,7 @@ async function startMcpHttpServer(ctx) { GetIssueCommentsTool(ctx), GetIssueEventsTool(ctx), PullRequestTool(ctx), - // ReviewTool(ctx), - StartReviewTool(ctx), - AddReviewCommentTool(ctx), - SubmitReviewTool(ctx), + CreatePullRequestReviewTool(ctx), PullRequestInfoTool(ctx), CheckoutPrTool(ctx), GetReviewCommentsTool(ctx), diff --git a/mcp/checkout.ts b/mcp/checkout.ts index f4bcacb..8e32a38 100644 --- a/mcp/checkout.ts +++ b/mcp/checkout.ts @@ -180,7 +180,13 @@ export function CheckoutPrTool(ctx: ToolContext) { // write diff to file for grep access const diffContent = diffResponse.data as unknown as string; - const diffPath = join(process.env.PULLFROG_TEMP_DIR!, `pr-${pull_number}.diff`); + const tempDir = process.env.PULLFROG_TEMP_DIR; + if (!tempDir) { + throw new Error( + "PULLFROG_TEMP_DIR not set - checkout_pr must run in pullfrog action context" + ); + } + const diffPath = join(tempDir, `pr-${pull_number}.diff`); writeFileSync(diffPath, diffContent); log.debug(`wrote diff to ${diffPath} (${diffContent.length} bytes)`); diff --git a/mcp/review.ts b/mcp/review.ts index 3a8ae7f..2e10a5f 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -6,16 +6,150 @@ import { log } from "../utils/cli.ts"; import { deleteProgressComment } from "./comment.ts"; import { execute, tool } from "./shared.ts"; +// one-shot review tool +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." + ) + .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)" + ), + side: type + .enumerated("LEFT", "RIGHT") + .describe( + "Side of the diff: LEFT (old code) or RIGHT (new code). Defaults to RIGHT if not provided." + ) + .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)." + ) + .optional(), +}); + +export function CreatePullRequestReviewTool(ctx: ToolContext) { + 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.", + parameters: CreatePullRequestReview, + execute: execute(async ({ pull_number, body, commit_id, comments = [] }) => { + // set PR context + ctx.toolState.prNumber = pull_number; + + // get the PR to determine the head commit if commit_id not provided + const pr = await ctx.octokit.rest.pulls.get({ + owner: ctx.owner, + repo: ctx.name, + pull_number, + }); + + // compose the request + const params: RestEndpointMethodTypes["pulls"]["createReview"]["parameters"] = { + owner: ctx.owner, + repo: ctx.name, + pull_number, + event: "COMMENT", + }; + if (body) params.body = body; + if (commit_id) { + params.commit_id = commit_id; + } else { + params.commit_id = pr.data.head.sha; + } + if (comments.length > 0) { + type ReviewComment = (typeof params.comments & {})[number]; + // convert comments to the format expected by GitHub API + params.comments = comments.map((comment) => { + const reviewComment: ReviewComment = { + ...comment, + }; + reviewComment.side = comment.side || "RIGHT"; + if (comment.start_line) { + reviewComment.start_line = comment.start_line; + reviewComment.start_side = comment.side || "RIGHT"; + } + return reviewComment; + }); + } + const result = await ctx.octokit.rest.pulls.createReview(params); + log.debug(`createReview response: ${JSON.stringify(result.data)}`); + if (!result.data.id) { + throw new Error(`createReview returned invalid data: ${JSON.stringify(result.data)}`); + } + const reviewId = result.data.id; + + // build quick links footer and update the review body + const apiUrl = process.env.API_URL || "https://pullfrog.com"; + const fixAllUrl = `${apiUrl}/trigger/${ctx.owner}/${ctx.name}/${pull_number}?action=fix&review_id=${reviewId}`; + const fixApprovedUrl = `${apiUrl}/trigger/${ctx.owner}/${ctx.name}/${pull_number}?action=fix-approved&review_id=${reviewId}`; + + const footer = buildPullfrogFooter({ + workflowRun: { owner: ctx.owner, repo: ctx.name, runId: ctx.runId, jobId: ctx.jobId }, + customParts: [`[Fix all ➔](${fixAllUrl})`, `[Fix 👍s ➔](${fixApprovedUrl})`], + }); + + const updatedBody = (body || "") + footer; + + // update the review with the footer + await ctx.octokit.rest.pulls.updateReview({ + owner: ctx.owner, + repo: ctx.name, + pull_number, + review_id: reviewId, + body: updatedBody, + }); + + await deleteProgressComment(ctx); + + return { + success: true, + reviewId, + html_url: result.data.html_url, + state: result.data.state, + user: result.data.user?.login, + submitted_at: result.data.submitted_at, + }; + }), + }); +} + +// ============================================================================= +// COMMENTED OUT: Three-step review flow (start_review, add_review_comment, submit_review) +// This approach used GraphQL to add comments to a pending review one-by-one, +// but GitHub's API was returning null for valid lines. Keeping for reference. +// ============================================================================= + +/* // graphql mutation to add a comment thread to a pending review // note: REST API doesn't support adding comments to an existing pending review const ADD_PULL_REQUEST_REVIEW_THREAD = ` -mutation AddPullRequestReviewThread($pullRequestReviewId: ID!, $path: String!, $line: Int!, $body: String!, $side: DiffSide) { +mutation AddPullRequestReviewThread($pullRequestReviewId: ID!, $path: String!, $line: Int!, $body: String!, $side: DiffSide, $subjectType: PullRequestReviewThreadSubjectType) { addPullRequestReviewThread(input: { pullRequestReviewId: $pullRequestReviewId, path: $path, line: $line, body: $body, - side: $side + side: $side, + subjectType: $subjectType }) { thread { id @@ -180,6 +314,7 @@ export function AddReviewCommentTool(ctx: ToolContext) { line, body, side: side || "RIGHT", + subjectType: "LINE", } ); log.debug(`addPullRequestReviewThread response: ${JSON.stringify(result)}`); @@ -293,130 +428,4 @@ export function SubmitReviewTool(ctx: ToolContext) { }), }); } - -// legacy tool - kept for backwards compatibility -export const Review = 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." - ) - .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)" - ), - side: type - .enumerated("LEFT", "RIGHT") - .describe( - "Side of the diff: LEFT (old code) or RIGHT (new code). Defaults to RIGHT if not provided." - ) - .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)." - ) - .optional(), -}); - -export function ReviewTool(ctx: ToolContext) { - 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.", - parameters: Review, - execute: execute(async ({ pull_number, body, commit_id, comments = [] }) => { - // set PR context - ctx.toolState.prNumber = pull_number; - - // get the PR to determine the head commit if commit_id not provided - const pr = await ctx.octokit.rest.pulls.get({ - owner: ctx.owner, - repo: ctx.name, - pull_number, - }); - - // compose the request - const params: RestEndpointMethodTypes["pulls"]["createReview"]["parameters"] = { - owner: ctx.owner, - repo: ctx.name, - pull_number, - event: "COMMENT", - }; - if (body) params.body = body; - if (commit_id) { - params.commit_id = commit_id; - } else { - params.commit_id = pr.data.head.sha; - } - if (comments.length > 0) { - type ReviewComment = (typeof params.comments & {})[number]; - // convert comments to the format expected by GitHub API - params.comments = comments.map((comment) => { - const reviewComment: ReviewComment = { - ...comment, - }; - reviewComment.side = comment.side || "RIGHT"; - if (comment.start_line) { - reviewComment.start_line = comment.start_line; - reviewComment.start_side = comment.side || "RIGHT"; - } - return reviewComment; - }); - } - const result = await ctx.octokit.rest.pulls.createReview(params); - log.debug(`createReview (legacy) response: ${JSON.stringify(result.data)}`); - if (!result.data.id) { - throw new Error(`createReview returned invalid data: ${JSON.stringify(result.data)}`); - } - const reviewId = result.data.id; - - // build quick links footer and update the review body - const apiUrl = process.env.API_URL || "https://pullfrog.com"; - const fixAllUrl = `${apiUrl}/trigger/${ctx.owner}/${ctx.name}/${pull_number}?action=fix&review_id=${reviewId}`; - const fixApprovedUrl = `${apiUrl}/trigger/${ctx.owner}/${ctx.name}/${pull_number}?action=fix-approved&review_id=${reviewId}`; - - const footer = buildPullfrogFooter({ - workflowRun: { owner: ctx.owner, repo: ctx.name, runId: ctx.runId, jobId: ctx.jobId }, - customParts: [`[Fix all ➔](${fixAllUrl})`, `[Fix 👍s ➔](${fixApprovedUrl})`], - }); - - const updatedBody = (body || "") + footer; - - // update the review with the footer - await ctx.octokit.rest.pulls.updateReview({ - owner: ctx.owner, - repo: ctx.name, - pull_number, - review_id: reviewId, - body: updatedBody, - }); - - await deleteProgressComment(ctx); - - return { - success: true, - reviewId, - html_url: result.data.html_url, - state: result.data.state, - user: result.data.user?.login, - submitted_at: result.data.submitted_at, - }; - }), - }); -} +*/ diff --git a/mcp/server.ts b/mcp/server.ts index 698d7e6..63010da 100644 --- a/mcp/server.ts +++ b/mcp/server.ts @@ -26,7 +26,7 @@ import { IssueInfoTool } from "./issueInfo.ts"; import { AddLabelsTool } from "./labels.ts"; import { PullRequestTool } from "./pr.ts"; import { PullRequestInfoTool } from "./prInfo.ts"; -import { AddReviewCommentTool, StartReviewTool, SubmitReviewTool } from "./review.ts"; +import { CreatePullRequestReviewTool } from "./review.ts"; import { GetReviewCommentsTool, ListPullRequestReviewsTool } from "./reviewComments.ts"; import { SelectModeTool } from "./selectMode.ts"; import { addTools } from "./shared.ts"; @@ -84,10 +84,7 @@ export async function startMcpHttpServer( GetIssueCommentsTool(ctx), GetIssueEventsTool(ctx), PullRequestTool(ctx), - // ReviewTool(ctx), - StartReviewTool(ctx), - AddReviewCommentTool(ctx), - SubmitReviewTool(ctx), + CreatePullRequestReviewTool(ctx), PullRequestInfoTool(ctx), CheckoutPrTool(ctx), GetReviewCommentsTool(ctx), diff --git a/modes.ts b/modes.ts index bff3adb..126cb70 100644 --- a/modes.ts +++ b/modes.ts @@ -100,36 +100,38 @@ ${ description: "Review code, PRs, or implementations; provide feedback or suggestions; identify issues; or check code quality, style, and correctness", prompt: `Follow these steps: -1. Checkout the PR using ${ghPullfrogMcpName}/checkout_pr with the PR number. This fetches the PR branch and returns the PR diff in the \`diff\` field of the response. Use this diff for your review - it shows exactly what's in the PR (fetched via GitHub API, so it's not affected by main advancing after the branch was created). -2. Start review session using ${ghPullfrogMcpName}/start_review. This creates a pending review on GitHub and returns analysis guidance. Follow the guidance before adding comments. +1. **CHECKOUT** - Use ${ghPullfrogMcpName}/checkout_pr with the PR number. This fetches the PR branch and returns the diff. Use this diff for your review - it shows exactly what's in the PR. -3. **ANALYZE** - Before adding any comments, think through: +2. **UNDERSTAND CONTEXT** - Read the modified files to understand the changes in context. Don't just look at the diff - understand how the changes affect the overall codebase. + +3. **ANALYZE** - Think through: - What does this PR change? Summarize in 1-2 sentences. - - Is the approach sound? If not, **stop here** and comment on the approach first. Don't waste time on implementation details if the approach is wrong. + - 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. **BEFORE COMMENTING** - For each potential comment, ask yourself: - - Is this a nitpick? Skip it unless explicitly requested. - - Would the codebase maintainer care about this feedback, based on what you can infer about the code quality standards in this repo? +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 -5. Add inline review comments one-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") +5. **SELF-CRITIQUE** - Before submitting, review your draft: + - Remove nitpicks unless the user explicitly told you to be nitpicky. Nitpicks may include: requesting documentation/docstrings/JSDoc, commenting on minor code/whitespace formatting, commenting on small changes unrelated to the main changes. + - Ensure each comment is actionable - would the author know exactly what to do? + - Would the codebase maintainer care about this feedback? + - If you have approach-level concerns, consider whether implementation-level comments are worth including + - For issues appearing in multiple places, keep only the FIRST occurrence and reference others (e.g., "also at lines X, Y") -6. 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) +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) -**GENERAL GUIDANCE** - -- Do not leave any comments that are not potentially actionable. Do not leave complimentary comments just to be nice. -- 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 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. - `, +**CRITICAL RULES** +- 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 +`, }, { name: "Plan", diff --git a/package.json b/package.json index 432cc7f..5382ad6 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@pullfrog/action", - "version": "0.0.152", + "version": "0.0.153", "type": "module", "files": [ "index.js",