From 8cee07d388b736e4b87d9db1bb502a5834f07ed0 Mon Sep 17 00:00:00 2001
From: Colin McDonnell <colinmcd94@gmail.com>
Date: Mon, 4 May 2026 19:20:30 +0000
Subject: [PATCH] move progress-comment cleanup into create_pull_request_review
 (#551)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix: snapshot review state so progress comment cleanup actually fires

postReviewCleanup deletes toolState.review as its second statement, so
the defense-in-depth `if (toolState.review && progressCommentId)` branch
right after never saw a truthy value. This left an orphaned progress
comment alongside the submitted review whenever the agent called
report_progress despite Review/IncrementalReview mode instructions
(seen in the wild on colinhacks/zod#5767).

Snapshot the boolean before postReviewCleanup runs.

* move progress-comment cleanup into create_pull_request_review

The previous commit snapshotted toolState.review to work around
postReviewCleanup deleting it before the cleanup branch could read it.
That fixed the symptom but kept a fragile design: the rule "review
submitted → progress comment is noise" was enforced from the bottom of
main.ts via a flag set in one place and consumed in another, with a
helper between them that mutated the same flag for unrelated reasons.

Move the rule to its natural owner. create_pull_request_review now
calls deleteProgressComment immediately after the review is persisted,
so the cleanup is atomic with submission. This:

- closes the catch-block hole — a review submitted right before a
  timeout/crash now still cleans up its progress comment.
- removes the dead "defense-in-depth" branch in main.ts that was the
  original bug surface.
- relies on the existing progressCommentId=null no-op path in
  reportProgress to make any later report_progress call a no-op (so
  the misbehavior path can't re-create the orphan).
- only fires for Review/IncrementalReview in practice — those are the
  only modes that call create_pull_request_review, and both are
  prompted not to call report_progress. Build/AddressReviews/Plan
  never reach this code path, so their progress comments remain
  untouched.

Stranded-comment cleanup in main.ts is unchanged and still handles
the truly orphaned case (no review, no report_progress).
---
 main.ts       | 16 +++++-----------
 mcp/review.ts | 12 ++++++++++++
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/main.ts b/main.ts
index 775e6de..d1a0c86 100644
--- a/main.ts
+++ b/main.ts
@@ -529,23 +529,17 @@ export async function main(): Promise<MainResult> {
     // post-agent review cleanup: reportReviewNodeId → follow-up re-review dispatch.
     // runs after the agent exits so ordering is architecturally guaranteed (no LLM involvement).
     // best-effort: cleanup failures must not turn a successful agent run into a failure.
+    //
+    // note: progress-comment deletion on review submission is owned by
+    // create_pull_request_review (action/mcp/review.ts) and runs atomically
+    // with the submission, so it survives any path out of main (success,
+    // timeout, crash) without relying on cleanup ordering here.
     if (toolContext) {
       await postReviewCleanup(toolContext).catch((error) => {
         log.debug(`post-review cleanup failed: ${error}`);
       });
     }
 
-    // review submitted → always delete the progress comment.
-    // the review is the durable artifact; the progress comment is noise.
-    // defense-in-depth: covers the case where the agent calls report_progress
-    // despite mode instructions, which sets finalSummaryWritten and prevents
-    // the stranded-comment heuristic below from firing.
-    if (toolContext && toolState.review && toolState.progressCommentId) {
-      await deleteProgressComment(toolContext).catch((error) => {
-        log.debug(`review progress comment cleanup failed: ${error}`);
-      });
-    }
-
     // 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
diff --git a/mcp/review.ts b/mcp/review.ts
index e89fabb..6eb56a5 100644
--- a/mcp/review.ts
+++ b/mcp/review.ts
@@ -11,6 +11,7 @@ import {
 } from "../utils/diffCoverage.ts";
 import { fixDoubleEscapedString } from "../utils/fixDoubleEscapedString.ts";
 import { patchWorkflowRunFields } from "../utils/patchWorkflowRunFields.ts";
+import { deleteProgressComment } from "./comment.ts";
 import type { ToolContext } from "./server.ts";
 import { execute, tool } from "./shared.ts";
 
@@ -464,6 +465,17 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) {
         reviewedSha: actuallyReviewedSha,
       };
 
+      // 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,
+      // crash). deleteProgressComment sets progressCommentId = null, so a later
+      // report_progress call short-circuits to a no-op.
+      // best-effort: a cleanup failure must not turn a successful review into
+      // a tool-call failure visible to the agent.
+      await deleteProgressComment(ctx).catch((err) => {
+        log.debug(`progress comment cleanup after review failed: ${err}`);
+      });
+
       // detect commits pushed since checkout and guide the agent to review them
       // inline instead of dispatching a separate workflow run
       if (