From 4c1413d925ce2697a4c9652e6f6cd4626e9aca4e Mon Sep 17 00:00:00 2001 From: "pullfrog[bot]" <226033991+pullfrog[bot]@users.noreply.github.com> Date: Wed, 6 May 2026 21:05:53 +0000 Subject: [PATCH] fix(action): flip wasUpdated on substantive MCP write tools (#563) (#577) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(action): flip wasUpdated on substantive MCP write tools (#563) Review/Respond/etc. agents that submit a `create_pull_request_review`, `create_issue_comment`, or `update_pull_request_body` and exit without calling `report_progress` were being marked as workflow failures by the strict completion check in handleAgentResult. Extend the set of tools that flip toolState.wasUpdated so a substantive user-visible artifact satisfies the check. The isReviewMode bypass is retained for IncrementalReview's non-substantive path. Flag is set BEFORE patchWorkflowRunFields / deleteProgressComment in each tool so a best-effort cleanup failure does not undo the signal. * fix(action): use finalSummaryWritten for stranded progress cleanup The stranded-progress-comment cleanup at the end of main() previously fired only when toolState.wasUpdated was false (or the tracker was the last writer). With wasUpdated now set by additional MCP write tools (create_issue_comment, update_pull_request_body), an agent that produced a substantive artifact via one of those tools and skipped report_progress would leave the placeholder "Leaping into action" comment intact — the post-script then converted it into an error message on a successful run. Key the cleanup off finalSummaryWritten instead. That flag is only set when report_progress actually wrote the progress comment, so it cleanly distinguishes "comment is finalized" from "agent did other work but never touched the progress comment". * refactor(mcp): extract markSubstantiveArtifact() helper replaces 4 inline `ctx.toolState.wasUpdated = true` flips in CreateCommentTool, UpdatePullRequestBodyTool, and CreatePullRequestReviewTool with a single helper in mcp/server.ts. JSDoc on the helper documents the contract (call BEFORE downstream patch/cleanup; gates the strict completion check and stranded-comment cleanup) so future MCP write tool authors only need to grep for one symbol. no behavioral change. Co-authored-by: Cursor * fix(mcp): only flip finalSummaryWritten after non-skipped write Previously the flag was set unconditionally on any non-plan call, including paths where reportProgress skipped (silent events, deleted comment, no issue/PR target). The cleanup check in main.ts is safeguarded by toolState.progressComment so the bug doesn't manifest today, but aligning the flag with actual writes matches the wasUpdated pattern and the design intent in the cleanup plan. * refactor(mcp): inline markSubstantiveArtifact helper --------- Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> Co-authored-by: David Blass Co-authored-by: Cursor --- main.ts | 29 +++++++++++++++-------------- mcp/comment.ts | 12 ++++++++---- mcp/pr.ts | 2 ++ mcp/review.ts | 2 ++ utils/run.ts | 16 ++++++++-------- 5 files changed, 35 insertions(+), 26 deletions(-) diff --git a/main.ts b/main.ts index 3463ea2..3960f0d 100644 --- a/main.ts +++ b/main.ts @@ -705,20 +705,21 @@ export async function main(): Promise { }); } - // clean up stranded progress comments. two cases: - // 1. wasUpdated=false: nothing wrote to the comment ("Leaping into action" orphan) - // 2. tracker published a checklist but the agent never wrote a final summary - // (hasPublished=true, finalSummaryWritten=false). - // in both cases, delete the comment so it doesn't linger with stale content. - // wasUpdated is intentionally NOT set here — cleanup is not a real progress update. - // uses finalSummaryWritten (not todoTracker.enabled) so cleanup survives API failures - // in report_progress where cancel() ran but the write didn't succeed. - const trackerWasLastWriter = todoTracker?.hasPublished && !toolState.finalSummaryWritten; - if ( - toolContext && - toolState.progressComment && - (!toolState.wasUpdated || trackerWasLastWriter) - ) { + // clean up stranded progress comments. the comment is stale unless + // report_progress wrote a final summary to it — three sub-cases all reduce + // to !finalSummaryWritten: + // 1. nothing wrote to the comment ("Leaping into action" orphan) + // 2. tracker published a checklist but the agent never finalized it + // 3. the agent produced a substantive artifact via another MCP write tool + // (create_issue_comment, update_pull_request_body, reply_to_review_comment) + // and skipped report_progress — wasUpdated is true, but the progress + // comment itself was never touched. + // create_pull_request_review owns its own deletion (see action/mcp/review.ts), + // so progressComment is already null by the time we get here for that path. + // uses finalSummaryWritten (not todoTracker.enabled or wasUpdated) so cleanup + // survives API failures in report_progress where cancel() ran but the write + // didn't succeed, and isn't fooled by writes to *other* artifacts. + if (toolContext && toolState.progressComment && !toolState.finalSummaryWritten) { await deleteProgressComment(toolContext).catch((error) => { log.debug(`stranded progress comment cleanup failed: ${error}`); }); diff --git a/mcp/comment.ts b/mcp/comment.ts index c70a07f..a797375 100644 --- a/mcp/comment.ts +++ b/mcp/comment.ts @@ -91,6 +91,8 @@ export function CreateCommentTool(ctx: ToolContext) { body: bodyWithFooter, }); + ctx.toolState.wasUpdated = true; + if (result.data.node_id) { await patchWorkflowRunFields(ctx, { summaryCommentNodeId: result.data.node_id }); } @@ -110,6 +112,8 @@ export function CreateCommentTool(ctx: ToolContext) { body: bodyWithFooter, }); + ctx.toolState.wasUpdated = true; + if (commentType === "Plan") { if (result.data.node_id) { await patchWorkflowRunFields(ctx, { planCommentNodeId: result.data.node_id }); @@ -366,10 +370,6 @@ export function ReportProgressTool(ctx: ToolContext) { } const result = await reportProgress(ctx, reportParams); - if (!params.target_plan_comment) { - ctx.toolState.finalSummaryWritten = true; - } - if (result.action === "skipped") { return { success: true, @@ -378,6 +378,10 @@ export function ReportProgressTool(ctx: ToolContext) { }; } + if (!params.target_plan_comment) { + ctx.toolState.finalSummaryWritten = true; + } + return { success: true, ...result, diff --git a/mcp/pr.ts b/mcp/pr.ts index 841b21e..9110914 100644 --- a/mcp/pr.ts +++ b/mcp/pr.ts @@ -49,6 +49,8 @@ export function UpdatePullRequestBodyTool(ctx: ToolContext) { body: bodyWithFooter, }); + ctx.toolState.wasUpdated = true; + return { success: true, number: result.data.number, diff --git a/mcp/review.ts b/mcp/review.ts index dedbf71..3b10cb8 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -537,6 +537,8 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { reviewedSha: actuallyReviewedSha, }; + ctx.toolState.wasUpdated = true; + // a submitted review obsoletes the progress comment — the review IS the // durable artifact. owned here (not in main.ts) so cleanup is atomic with // submission and survives any path out of the run (success, timeout, diff --git a/utils/run.ts b/utils/run.ts index b1cc201..6397cbb 100644 --- a/utils/run.ts +++ b/utils/run.ts @@ -19,14 +19,14 @@ export async function handleAgentResult(ctx: HandleAgentResultParams): Promise