move progress-comment cleanup into create_pull_request_review (#551)
* 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).
This commit is contained in:
committed by
pullfrog[bot]
parent
b835d53d83
commit
8cee07d388
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user