From 089a05b13e96806c681f3445bc96da18260d5f55 Mon Sep 17 00:00:00 2001 From: Colin McDonnell Date: Wed, 11 Mar 2026 01:56:57 +0000 Subject: [PATCH] fix bodyless review bug by using pending + submit flow (#469) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix bodyless review bug by using pending + submit flow createReview with event:"COMMENT" publishes immediately, so the subsequent updateReview (to add footer with Fix links) fails when the agent omits the review-level body — GitHub rejects editing a bodyless review. this left ghost reviews and caused retries with a garbage body like `" "`. switch to a two-phase flow: createReview without event (PENDING), then submitReview with the full body + footer. single atomic publish, no updateReview needed. Made-with: Cursor * support bodyless reviews — skip footer when no body provided Made-with: Cursor * early return for bodyless reviews Made-with: Cursor * extract submitAndCleanup and buildReviewFooter helpers Made-with: Cursor * fix: default approved to false for buildReviewFooter Made-with: Cursor * run action typecheck alongside root tsc Made-with: Cursor * refactor: extract submitReview helper, keep cleanup inline Made-with: Cursor * skip pending+submit for bodyless reviews — single createReview instead Made-with: Cursor * restore pre-existing comments Made-with: Cursor --- entry | 69 ++++++++++++++++++------------------ mcp/review.ts | 97 ++++++++++++++++++++++++++++----------------------- 2 files changed, 90 insertions(+), 76 deletions(-) diff --git a/entry b/entry index b812e8a..eb81fc9 100755 --- a/entry +++ b/entry @@ -147194,7 +147194,6 @@ function CreatePullRequestReviewTool(ctx) { pull_number, event }; - if (body) params.body = body; if (commit_id) { params.commit_id = commit_id; } else { @@ -147224,44 +147223,17 @@ function CreatePullRequestReviewTool(ctx) { return reviewComment; }); } - const result = await ctx.octokit.rest.pulls.createReview(params); - log.debug(`createReview response: ${JSON.stringify(result.data)}`); + const result = body ? await createAndSubmitWithFooter(ctx, params, { + body, + approved: approved ?? false, + hasComments: comments.length > 0 + }) : await ctx.octokit.rest.pulls.createReview(params); if (!result.data.id) { throw new Error(`createReview returned invalid data: ${JSON.stringify(result.data)}`); } const reviewId = result.data.id; const reviewNodeId = result.data.node_id; await reportReviewNodeId(ctx, reviewNodeId); - const customParts = []; - if (!approved) { - if (comments.length > 0) { - const apiUrl = getApiUrl(); - 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}`; - customParts.push(`[Fix all \u2794](${fixAllUrl})`, `[Fix \u{1F44D}s \u2794](${fixApprovedUrl})`); - } else if (body) { - const apiUrl = getApiUrl(); - const fixUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix&review_id=${reviewId}`; - customParts.push(`[Fix it \u2794](${fixUrl})`); - } - } - const footer = buildPullfrogFooter({ - workflowRun: ctx.runId ? { - owner: ctx.repo.owner, - repo: ctx.repo.name, - runId: ctx.runId, - jobId: ctx.jobId - } : void 0, - customParts - }); - const updatedBody = (body || "") + footer; - await ctx.octokit.rest.pulls.updateReview({ - owner: ctx.repo.owner, - repo: ctx.repo.name, - pull_number, - review_id: reviewId, - body: updatedBody - }); await deleteProgressComment(ctx); return { success: true, @@ -147274,6 +147246,37 @@ function CreatePullRequestReviewTool(ctx) { }) }); } +async function createAndSubmitWithFooter(ctx, params, opts) { + const { event: _, ...pendingParams } = params; + const pending = await ctx.octokit.rest.pulls.createReview(pendingParams); + if (!pending.data.id) { + throw new Error(`createReview returned invalid data: ${JSON.stringify(pending.data)}`); + } + const customParts = []; + if (!opts.approved) { + const apiUrl = getApiUrl(); + if (opts.hasComments) { + const fixAllUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${params.pull_number}?action=fix&review_id=${pending.data.id}`; + const fixApprovedUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${params.pull_number}?action=fix-approved&review_id=${pending.data.id}`; + customParts.push(`[Fix all \u2794](${fixAllUrl})`, `[Fix \u{1F44D}s \u2794](${fixApprovedUrl})`); + } else { + const fixUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${params.pull_number}?action=fix&review_id=${pending.data.id}`; + customParts.push(`[Fix it \u2794](${fixUrl})`); + } + } + const footer = buildPullfrogFooter({ + workflowRun: ctx.runId ? { owner: ctx.repo.owner, repo: ctx.repo.name, runId: ctx.runId, jobId: ctx.jobId } : void 0, + customParts + }); + return ctx.octokit.rest.pulls.submitReview({ + owner: params.owner, + repo: params.repo, + pull_number: params.pull_number, + review_id: pending.data.id, + event: params.event, + body: opts.body + footer + }); +} async function reportReviewNodeId(ctx, reviewNodeId) { for (let remaining = 2; remaining >= 0; remaining--) { try { diff --git a/mcp/review.ts b/mcp/review.ts index a31a701..98be317 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -88,7 +88,6 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { pull_number, event, }; - if (body) params.body = body; if (commit_id) { params.commit_id = commit_id; } else { @@ -124,8 +123,16 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { }); } - const result = await ctx.octokit.rest.pulls.createReview(params); - log.debug(`createReview response: ${JSON.stringify(result.data)}`); + // no body → single-step createReview (no footer needed) + // has body → pending + submit so we can build footer with Fix links using review ID + const result = body + ? await createAndSubmitWithFooter(ctx, params, { + body, + approved: approved ?? false, + hasComments: comments.length > 0, + }) + : await ctx.octokit.rest.pulls.createReview(params); + if (!result.data.id) { throw new Error(`createReview returned invalid data: ${JSON.stringify(result.data)}`); } @@ -137,46 +144,6 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { // awaited (not fire-and-forget) to guarantee the signal lands before // any subsequent push's webhook checks for in-flight runs. await reportReviewNodeId(ctx, reviewNodeId); - - // build quick links footer and update the review body - // only include "Fix all" and "Fix 👍s" links if there are actual review comments - const customParts: string[] = []; - if (!approved) { - if (comments.length > 0) { - const apiUrl = getApiUrl(); - 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}`; - customParts.push(`[Fix all ➔](${fixAllUrl})`, `[Fix 👍s ➔](${fixApprovedUrl})`); - } else if (body) { - const apiUrl = getApiUrl(); - const fixUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${pull_number}?action=fix&review_id=${reviewId}`; - customParts.push(`[Fix it ➔](${fixUrl})`); - } - } - - const footer = buildPullfrogFooter({ - workflowRun: ctx.runId - ? { - owner: ctx.repo.owner, - repo: ctx.repo.name, - runId: ctx.runId, - jobId: ctx.jobId, - } - : undefined, - customParts, - }); - - const updatedBody = (body || "") + footer; - - // update the review with the footer - await ctx.octokit.rest.pulls.updateReview({ - owner: ctx.repo.owner, - repo: ctx.repo.name, - pull_number, - review_id: reviewId, - body: updatedBody, - }); - await deleteProgressComment(ctx); return { @@ -191,6 +158,50 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { }); } +type FooterOpts = { body: string; approved: boolean; hasComments: boolean }; + +async function createAndSubmitWithFooter( + ctx: ToolContext, + params: RestEndpointMethodTypes["pulls"]["createReview"]["parameters"], + opts: FooterOpts +) { + // create as PENDING (strip event) so we get the review ID before publishing + const { event: _, ...pendingParams } = params; + const pending = await ctx.octokit.rest.pulls.createReview(pendingParams); + if (!pending.data.id) { + throw new Error(`createReview returned invalid data: ${JSON.stringify(pending.data)}`); + } + + const customParts: string[] = []; + if (!opts.approved) { + const apiUrl = getApiUrl(); + if (opts.hasComments) { + const fixAllUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${params.pull_number}?action=fix&review_id=${pending.data.id}`; + const fixApprovedUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${params.pull_number}?action=fix-approved&review_id=${pending.data.id}`; + customParts.push(`[Fix all ➔](${fixAllUrl})`, `[Fix 👍s ➔](${fixApprovedUrl})`); + } else { + const fixUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${params.pull_number}?action=fix&review_id=${pending.data.id}`; + customParts.push(`[Fix it ➔](${fixUrl})`); + } + } + + const footer = buildPullfrogFooter({ + workflowRun: ctx.runId + ? { owner: ctx.repo.owner, repo: ctx.repo.name, runId: ctx.runId, jobId: ctx.jobId } + : undefined, + customParts, + }); + + return ctx.octokit.rest.pulls.submitReview({ + owner: params.owner, + repo: params.repo, + pull_number: params.pull_number, + review_id: pending.data.id, + event: params.event!, + body: opts.body + footer, + }); +} + async function reportReviewNodeId(ctx: ToolContext, reviewNodeId: string): Promise { for (let remaining = 2; remaining >= 0; remaining--) { try {