From e24d19442ff1d5c2e26ff25d078a57fc791c25a7 Mon Sep 17 00:00:00 2001 From: Ismael Martinez Ramos Date: Tue, 19 May 2026 17:06:23 +0100 Subject: [PATCH 1/5] fix(gitlab): keep persistent /improve thread stable when it has replies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In publish_persistent_comment_with_history, edit the previously-found persistent comment in place and remove the throwaway "Preparing suggestions..." progress note, instead of the other way around. The previous ordering targeted the progress note for the merged update and tried to delete the original; GitLabProvider.remove_comment silently no-ops when GitLab refuses to delete a note that has replies (HTTP 403), so any /improve run on an MR where the author had engaged with the suggestions thread left the original intact and promoted the progress note to a duplicate thread on every subsequent push. The original ordering was deliberate — a freshly-posted note refreshes in the GitLab UI faster than an edit of an older note — but that benefit is lost silently the moment the thread has a reply, and the duplicate-thread cost is much worse than the slower-UI cost. Refs The-PR-Agent/pr-agent#2402. --- pr_agent/tools/pr_code_suggestions.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/pr_agent/tools/pr_code_suggestions.py b/pr_agent/tools/pr_code_suggestions.py index 6372396c8b..38b62832e5 100644 --- a/pr_agent/tools/pr_code_suggestions.py +++ b/pr_agent/tools/pr_code_suggestions.py @@ -333,12 +333,13 @@ def _extract_link(comment_text: str): pr_comment_updated += f"{prev_suggestion_table}\n" get_logger().info(f"Persistent mode - updating comment {comment_url} to latest {name} message") - if progress_response: # publish to 'progress_response' comment, because it refreshes immediately - git_provider.edit_comment(progress_response, pr_comment_updated) - git_provider.remove_comment(comment) - comment = progress_response - else: - git_provider.edit_comment(comment, pr_comment_updated) + # Edit the previously-found persistent comment in place and remove the throwaway + # progress note. Editing the persistent comment (rather than re-targeting to the + # progress note and deleting the original) keeps the stable thread stable across + # pushes on GitLab, where deleting a note with replies fails silently. + git_provider.edit_comment(comment, pr_comment_updated) + if progress_response: + git_provider.remove_comment(progress_response) return comment except Exception as e: get_logger().exception(f"Failed to update persistent review, error: {e}") From 8f5279286592feaf4d1af6adcbc8aaa48954bd80 Mon Sep 17 00:00:00 2001 From: Ismael Martinez Ramos Date: Tue, 19 May 2026 17:17:09 +0100 Subject: [PATCH 2/5] fix(gitlab): replace progress note body before deletion Edit the progress note to a benign final-state message before attempting its deletion. The previous code relied on remove_comment succeeding to clean up the "Work in progress ..." body; if the delete fails for any reason (transient network errors, or GitLab silently swallowing a 403), the leftover note keeps displaying stale WIP text. Pre-editing to a short "Code suggestions published in the persistent thread above." message means a failed delete leaves a non-misleading breadcrumb instead. Addresses the Qodo review on PR #2404. --- pr_agent/tools/pr_code_suggestions.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pr_agent/tools/pr_code_suggestions.py b/pr_agent/tools/pr_code_suggestions.py index 38b62832e5..cdd12f0e25 100644 --- a/pr_agent/tools/pr_code_suggestions.py +++ b/pr_agent/tools/pr_code_suggestions.py @@ -339,6 +339,10 @@ def _extract_link(comment_text: str): # pushes on GitLab, where deleting a note with replies fails silently. git_provider.edit_comment(comment, pr_comment_updated) if progress_response: + # Replace the WIP progress body with a benign final-state message before + # deletion so that if remove_comment fails for any reason, the leftover note + # does not keep displaying "Work in progress ...". + git_provider.edit_comment(progress_response, "Code suggestions published in the persistent thread above.") git_provider.remove_comment(progress_response) return comment except Exception as e: From 8c000f125ae271d7bedc18706c2586fc4aa22b65 Mon Sep 17 00:00:00 2001 From: Ismael Martinez Ramos Date: Tue, 19 May 2026 17:28:23 +0100 Subject: [PATCH 3/5] fix(gitlab): isolate progress-note cleanup from persistent-update path Wrap the progress-note edit-and-delete cleanup in its own try/except so that an exception there (most realistically a GitLab edit_comment API failure, which is not caught by the provider) does not fall through to the outer exception handler and re-trigger the "no previous comment found" fallback below. The fallback path would then publish a new suggestions thread despite the persistent comment update at the top of the block having already succeeded, recreating the duplicate thread this PR is meant to prevent. Cleanup is best-effort and never affects the success of the persistent update. Addresses the Qodo follow-up review on PR #2404. --- pr_agent/tools/pr_code_suggestions.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/pr_agent/tools/pr_code_suggestions.py b/pr_agent/tools/pr_code_suggestions.py index cdd12f0e25..6c82ae37c3 100644 --- a/pr_agent/tools/pr_code_suggestions.py +++ b/pr_agent/tools/pr_code_suggestions.py @@ -339,11 +339,20 @@ def _extract_link(comment_text: str): # pushes on GitLab, where deleting a note with replies fails silently. git_provider.edit_comment(comment, pr_comment_updated) if progress_response: - # Replace the WIP progress body with a benign final-state message before - # deletion so that if remove_comment fails for any reason, the leftover note - # does not keep displaying "Work in progress ...". - git_provider.edit_comment(progress_response, "Code suggestions published in the persistent thread above.") - git_provider.remove_comment(progress_response) + # Cleanup is best-effort: isolate it from the outer try/except so a failure + # here does not trigger the "no previous comment" fallback below, which + # would publish a duplicate suggestions thread despite the persistent + # comment update having already succeeded. + try: + # Replace the WIP progress body with a benign final-state message + # before deletion so that if remove_comment fails for any reason, the + # leftover note does not keep displaying "Work in progress ...". + git_provider.edit_comment(progress_response, "Code suggestions published in the persistent thread above.") + git_provider.remove_comment(progress_response) + except Exception as cleanup_error: + get_logger().warning( + f"Failed to clean up progress note after persistent update, leaving it in place: {cleanup_error}" + ) return comment except Exception as e: get_logger().exception(f"Failed to update persistent review, error: {e}") From f0b597b6493e8aebc82bbd6390c8c6d309fd05a7 Mon Sep 17 00:00:00 2001 From: Ismael Martinez Ramos Date: Mon, 25 May 2026 07:17:31 +0100 Subject: [PATCH 4/5] refactor: remove verbose comments from persistent-comment cleanup The code is self-explanatory; the comments were restating what the edit/try/except structure already communicates. Co-Authored-By: Claude Opus 4.7 (1M context) --- pr_agent/tools/pr_code_suggestions.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/pr_agent/tools/pr_code_suggestions.py b/pr_agent/tools/pr_code_suggestions.py index 6c82ae37c3..1c4f457b31 100644 --- a/pr_agent/tools/pr_code_suggestions.py +++ b/pr_agent/tools/pr_code_suggestions.py @@ -333,20 +333,9 @@ def _extract_link(comment_text: str): pr_comment_updated += f"{prev_suggestion_table}\n" get_logger().info(f"Persistent mode - updating comment {comment_url} to latest {name} message") - # Edit the previously-found persistent comment in place and remove the throwaway - # progress note. Editing the persistent comment (rather than re-targeting to the - # progress note and deleting the original) keeps the stable thread stable across - # pushes on GitLab, where deleting a note with replies fails silently. git_provider.edit_comment(comment, pr_comment_updated) if progress_response: - # Cleanup is best-effort: isolate it from the outer try/except so a failure - # here does not trigger the "no previous comment" fallback below, which - # would publish a duplicate suggestions thread despite the persistent - # comment update having already succeeded. try: - # Replace the WIP progress body with a benign final-state message - # before deletion so that if remove_comment fails for any reason, the - # leftover note does not keep displaying "Work in progress ...". git_provider.edit_comment(progress_response, "Code suggestions published in the persistent thread above.") git_provider.remove_comment(progress_response) except Exception as cleanup_error: From bb435aec53d68579b8b6841c6f1e1d7d1825ed0e Mon Sep 17 00:00:00 2001 From: Ismael Martinez Ramos Date: Fri, 12 Jun 2026 10:19:31 +0100 Subject: [PATCH 5/5] test: cover progress-cleanup failure in persistent /improve update; document the intentional broad catch Co-Authored-By: Claude Fable 5 --- pr_agent/tools/pr_code_suggestions.py | 1 + .../unittest/test_pr_code_suggestions_core.py | 27 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/pr_agent/tools/pr_code_suggestions.py b/pr_agent/tools/pr_code_suggestions.py index 3b470ab462..5fbee9f202 100644 --- a/pr_agent/tools/pr_code_suggestions.py +++ b/pr_agent/tools/pr_code_suggestions.py @@ -335,6 +335,7 @@ def _extract_link(comment_text: str): get_logger().info(f"Persistent mode - updating comment {comment_url} to latest {name} message") git_provider.edit_comment(comment, pr_comment_updated) if progress_response: + # best-effort: propagating would re-trigger the duplicate-thread fallback below try: git_provider.edit_comment(progress_response, "Code suggestions published in the persistent thread above.") git_provider.remove_comment(progress_response) diff --git a/tests/unittest/test_pr_code_suggestions_core.py b/tests/unittest/test_pr_code_suggestions_core.py index 0522c1fb9f..1a20a9f74c 100644 --- a/tests/unittest/test_pr_code_suggestions_core.py +++ b/tests/unittest/test_pr_code_suggestions_core.py @@ -180,3 +180,30 @@ async def test_push_inline_code_suggestions_falls_back_to_individual_publish_cal assert second_retry[0]["relevant_lines_start"] == 2 assert second_retry[0]["relevant_lines_end"] == 2 assert "```suggestion\n return new_worker()" in second_retry[0]["body"] + + +def test_persistent_update_survives_progress_cleanup_failure(): + """A failing progress-note cleanup must not abort the persistent update: + if the cleanup error propagated, the caller would fall back to publishing + a new suggestions thread, re-creating the duplicate-thread bug.""" + initial_header = "## PR Code Suggestions" + existing = MagicMock() + existing.body = f"{initial_header}\n\nold suggestions
" + provider = MagicMock() + provider.get_issue_comments.return_value = [existing] + provider.get_comment_url.return_value = "https://example.test/comment/1" + provider.get_latest_commit_url.return_value = "https://example.test/commit/deadbee" + # First edit updates the persistent comment and succeeds; the second edit + # (re-labelling the progress note before deletion) fails. + provider.edit_comment.side_effect = [None, RuntimeError("cleanup failed")] + progress_note = MagicMock() + + result = PRCodeSuggestions.publish_persistent_comment_with_history( + provider, f"{initial_header}\nnew suggestions
", initial_header, + update_header=False, name="suggestions", final_update_message=False, + progress_response=progress_note) + + assert result is existing + assert provider.edit_comment.call_count == 2 + provider.remove_comment.assert_not_called() + provider.publish_comment.assert_not_called()