fix(action): flip wasUpdated on substantive MCP write tools (#563) (#577)

* 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 <cursoragent@cursor.com>

* 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 <david@arktype.io>
Co-authored-by: Cursor <cursoragent@cursor.com>

main.ts

@@ -705,20 +705,21 @@ export async function main(): Promise<MainResult> {
       });
     }
 
-    // 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}`);
       });

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,

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,

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,

utils/run.ts

@@ -19,14 +19,14 @@ export async function handleAgentResult(ctx: HandleAgentResultParams): Promise<M
     };
   }
 
-  // Review and IncrementalReview modes intentionally never set wasUpdated:
-  // the prompt forbids report_progress (the review IS the durable record),
-  // and IncrementalReview's non-substantive path produces no review at
-  // all by design. wasUpdated staying false is also load-bearing for the
-  // stranded-comment cleanup in main.ts which deletes the "Leaping into
-  // action" orphan via `(!wasUpdated || trackerWasLastWriter)`. Skip the
-  // strict completion check for these modes — the agent's exit code is
-  // the completion signal, not a progress-comment write.
+  // IncrementalReview's non-substantive path exits cleanly without
+  // submitting any review, so no MCP write tool flips wasUpdated and the
+  // strict completion check below would otherwise fail the run. The
+  // isReviewMode skip is load-bearing for that path: the agent's exit
+  // code is the completion signal, not a progress-comment write.
+  // (Review mode that submits a real review now flips wasUpdated via
+  // create_pull_request_review, so the skip is redundant for the
+  // substantive-review path but kept for symmetry with IncrementalReview.)
   // See plans/review_progress_comment_cleanup_b0120f6c.plan.md.
   const mode = ctx.toolState.selectedMode;
   const isReviewMode = mode === "Review" || mode === "IncrementalReview";