fix(gitlab): keep persistent /improve thread stable when it has replies

[IsmaelMartinez]

What

Flip the edit/delete pairing in publish_persistent_comment_with_history for the progress_response branch: edit the previously-found persistent comment in place and remove the throwaway progress note, instead of editing the progress note and trying to delete the original.

Why

Fixes #2402. On GitLab, the previous behaviour leaves a duplicate ## PR Code Suggestions ✨ thread on every push once the original has any reply, because GitLabProvider.remove_comment silently no-ops when GitLab refuses to delete a note with replies (HTTP 403). The fix keeps the original persistent thread stable across pushes; the progress note is throwaway and safe to delete (and harmless if its delete ever fails).

Changes

• pr_agent/tools/pr_code_suggestions.py — swap the targets of edit_comment and remove_comment in the if progress_response: branch.

Trade-off

The previous ordering was deliberate — the inline comment at the call site read # publish to 'progress_response' comment, because it refreshes immediately. The intent was that on GitLab a freshly-posted note renders in the UI more promptly than an edit of an older note. This PR gives that up on the happy path (no replies) in exchange for stability when replies are present. In practice GitLab does update edited notes, so the UX cost is small, but it is a real behavioural change rather than a pure bug fix. The duplicate-thread cost on engaged MRs seems clearly worse than the slower-refresh cost on quiet ones.

Tests

• Existing unit suite passes (PYTHONPATH=. ./.venv/bin/pytest tests/unittest -q — 390 passed).
• Manual repro on GitLab.com against a multi-push MR with replies on the suggestions thread:
  • Before: one new ## PR Code Suggestions ✨ discussion per push.
  • After: single discussion edited in place across pushes, with "Previous suggestions up to commit ..." accumulating as designed.

Risk

Scoped to the progress_response code path inside publish_persistent_comment_with_history. GitHub and other providers exercise the same branch but their remove_comment paths are stable for either ordering, so the swap is content-equivalent for them (the resulting comment ID changes, but the visible content does not).

A defence-in-depth follow-up — making GitLabProvider.remove_comment detect the "cannot delete a note with replies" case and skip the delete cleanly rather than swallowing the 403 — is purely additive and worth doing separately. Left out of this PR to keep it minimal.

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Fix GitLab duplicate suggestions thread with replies

🐞 Bug fix

Walkthroughs

Description

• Swap edit/delete targets in GitLab persistent comment handling
• Edit original persistent comment instead of progress note
• Remove throwaway progress note instead of original comment
• Prevents duplicate suggestion threads when replies exist

Diagram

flowchart LR
  A["progress_response exists"] -->|Before| B["Edit progress_response"]
  B --> C["Delete original comment"]
  C --> D["Duplicate thread on replies"]
  A -->|After| E["Edit original comment"]
  E --> F["Delete progress_response"]
  F --> G["Stable thread across pushes"]

Loading

File Changes

1. pr_agent/tools/pr_code_suggestions.py 🐞 Bug fix +7/-6

Swap edit/delete targets for persistent comment stability

• Swapped targets of edit_comment and remove_comment in the progress_response branch
• Now edits the original persistent comment and removes the throwaway progress note
• Updated inline comments to explain the GitLab-specific behavior and rationale
• Prevents silent failures when GitLab refuses to delete notes with replies (HTTP 403)

pr_agent/tools/pr_code_suggestions.py

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

✅ Tickets: 🎫 /improve on GitLab duplicates the persistent suggestions thread on every push once the previous one has any reply

1. Cleanup triggers duplicate thread ✓ Resolved 🐞 Bug ☼ Reliability

Description

In publish_persistent_comment_with_history, an exception while editing/deleting progress_response is
caught by the broad try/except and then execution falls through to the “did not find a previous
comment” path, which can publish a brand-new suggestions thread even if the persistent comment was
already updated. This is especially plausible on GitLab because GitLabProvider.edit_comment does not
catch exceptions, so an API failure during cleanup can reintroduce duplicate threads and/or throw
later when the fallback tries to edit the same progress note again.

Code

pr_agent/tools/pr_code_suggestions.py[R341-346]

+                        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)

Evidence

The cleanup operations are inside the same try/except as the persistent update; on any exception,
the function logs and then continues into the fallback branch that creates/edits a new comment. On
GitLab, edit_comment can throw because it directly calls the GitLab API without exception handling,
making this control-flow hazard realistic during transient API failures.

pr_agent/tools/pr_code_suggestions.py[335-360]
pr_agent/git_providers/gitlab_provider.py[523-526]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`publish_persistent_comment_with_history` performs progress-note cleanup (`edit_comment(progress_response, ...)` + `remove_comment(progress_response)`) inside the same broad `try:` block as the main persistent-comment update. If cleanup throws, the function logs and then falls through to the fallback path that assumes no previous persistent comment was found, potentially creating a new suggestions thread even though the persistent comment update already happened.
### Issue Context
- Some providers (notably GitLab) do not wrap `edit_comment` in a try/except, so transient API errors can throw from cleanup.
- Cleanup should be best-effort and must not affect the success path of updating the persistent thread.
### Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[335-360]
- pr_agent/git_providers/gitlab_provider.py[523-526]
### Suggested fix
1. Keep the main persistent update (`git_provider.edit_comment(comment, pr_comment_updated)`) as the decisive operation.
2. Wrap the progress-note cleanup in its own `try/except` (or otherwise ensure failures there do not enter the fallback “publish new comment” path). For example:
- Update persistent comment.
- If `progress_response`: try to edit it to benign text and remove it; on failure, log a warning and continue.
- Return the persistent `comment` regardless of cleanup outcome.
3. Optionally, also guard the later fallback block so it is only used when the persistent-comment edit truly failed, not when cleanup failed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Stale progress note linger ✓ Resolved 🐞 Bug ☼ Reliability

Description

When a prior persistent suggestions comment exists, the code now edits the persistent comment and
only attempts to delete the progress note; if that delete fails, the progress note remains with
“Generating PR code suggestions / Work in progress …”, which is misleading noise in the MR
discussion. This is especially relevant on GitLab where deletions can fail (e.g., notes with
replies) and the provider swallows the failure after logging it.

Code

pr_agent/tools/pr_code_suggestions.py[R340-342]

+                        git_provider.edit_comment(comment, pr_comment_updated)
+                        if progress_response:
+                            git_provider.remove_comment(progress_response)

Evidence

The updated persistent path edits only the persistent comment and then deletes progress_response
without ever updating its body; if deletion fails, the progress note content remains. The progress
note content explicitly indicates WIP, and GitLab’s remove implementation catches deletion
exceptions (so the failure can occur without preventing the run from completing).

pr_agent/tools/pr_code_suggestions.py[335-356]
pr_agent/tools/progress_comment.py[25-33]
pr_agent/git_providers/gitlab_provider.py[761-766]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
In the persistent-update path, `progress_response` is no longer edited; it is only deleted. If deletion fails, the MR keeps a stale “Generating PR code suggestions / Work in progress …” note.
### Issue Context
- The progress body is a WIP message.
- `GitLabProvider.remove_comment` catches and logs deletion errors, so failures won’t stop execution but can leave the WIP note behind.
### Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[335-356]
### Suggested fix
Before attempting deletion, edit `progress_response` to a short final state (e.g., “Suggestions published/updated in the persistent thread above.”) so that even if `remove_comment` fails, the leftover note is not misleading. Keep the existing delete attempt afterward.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 8c000f1

Results up to commit N/A

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Cleanup triggers duplicate thread ✓ Resolved 🐞 Bug ☼ Reliability

Description

In publish_persistent_comment_with_history, an exception while editing/deleting progress_response is
caught by the broad try/except and then execution falls through to the “did not find a previous
comment” path, which can publish a brand-new suggestions thread even if the persistent comment was
already updated. This is especially plausible on GitLab because GitLabProvider.edit_comment does not
catch exceptions, so an API failure during cleanup can reintroduce duplicate threads and/or throw
later when the fallback tries to edit the same progress note again.

Code

pr_agent/tools/pr_code_suggestions.py[R341-346]

+                        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)

Evidence

The cleanup operations are inside the same try/except as the persistent update; on any exception,
the function logs and then continues into the fallback branch that creates/edits a new comment. On
GitLab, edit_comment can throw because it directly calls the GitLab API without exception handling,
making this control-flow hazard realistic during transient API failures.

pr_agent/tools/pr_code_suggestions.py[335-360]
pr_agent/git_providers/gitlab_provider.py[523-526]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`publish_persistent_comment_with_history` performs progress-note cleanup (`edit_comment(progress_response, ...)` + `remove_comment(progress_response)`) inside the same broad `try:` block as the main persistent-comment update. If cleanup throws, the function logs and then falls through to the fallback path that assumes no previous persistent comment was found, potentially creating a new suggestions thread even though the persistent comment update already happened.
### Issue Context
- Some providers (notably GitLab) do not wrap `edit_comment` in a try/except, so transient API errors can throw from cleanup.
- Cleanup should be best-effort and must not affect the success path of updating the persistent thread.
### Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[335-360]
- pr_agent/git_providers/gitlab_provider.py[523-526]
### Suggested fix
1. Keep the main persistent update (`git_provider.edit_comment(comment, pr_comment_updated)`) as the decisive operation.
2. Wrap the progress-note cleanup in its own `try/except` (or otherwise ensure failures there do not enter the fallback “publish new comment” path). For example:
 - Update persistent comment.
 - If `progress_response`: try to edit it to benign text and remove it; on failure, log a warning and continue.
 - Return the persistent `comment` regardless of cleanup outcome.
3. Optionally, also guard the later fallback block so it is only used when the persistent-comment edit truly failed, not when cleanup failed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Stale progress note linger ✓ Resolved 🐞 Bug ☼ Reliability

Description

When a prior persistent suggestions comment exists, the code now edits the persistent comment and
only attempts to delete the progress note; if that delete fails, the progress note remains with
“Generating PR code suggestions / Work in progress …”, which is misleading noise in the MR
discussion. This is especially relevant on GitLab where deletions can fail (e.g., notes with
replies) and the provider swallows the failure after logging it.

Code

pr_agent/tools/pr_code_suggestions.py[R340-342]

+                        git_provider.edit_comment(comment, pr_comment_updated)
+                        if progress_response:
+                            git_provider.remove_comment(progress_response)

Evidence

The updated persistent path edits only the persistent comment and then deletes progress_response
without ever updating its body; if deletion fails, the progress note content remains. The progress
note content explicitly indicates WIP, and GitLab’s remove implementation catches deletion
exceptions (so the failure can occur without preventing the run from completing).

pr_agent/tools/pr_code_suggestions.py[335-356]
pr_agent/tools/progress_comment.py[25-33]
pr_agent/git_providers/gitlab_provider.py[761-766]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
In the persistent-update path, `progress_response` is no longer edited; it is only deleted. If deletion fails, the MR keeps a stale “Generating PR code suggestions / Work in progress …” note.
### Issue Context
- The progress body is a WIP message.
- `GitLabProvider.remove_comment` catches and logs deletion errors, so failures won’t stop execution but can leave the WIP note behind.
### Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[335-356]
### Suggested fix
Before attempting deletion, edit `progress_response` to a short final state (e.g., “Suggestions published/updated in the persistent thread above.”) so that even if `remove_comment` fails, the leftover note is not misleading. Keep the existing delete attempt afterward.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[IsmaelMartinez]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[IsmaelMartinez]

Stale progress note linger

Good catch — applied in 8f527928. Pre-edit the progress note body to "Code suggestions published in the persistent thread above." before attempting deletion, so a failed remove_comment (transient or silent-403) leaves a non-misleading breadcrumb instead of stale WIP text.

Scoped to the path Qodo flagged (publish_persistent_comment_with_history). The same pattern exists at two other call sites in pr_code_suggestions.py (post-inline-publish at L178 and the exception handler at L189) — those are pre-existing and out of scope for this fix.

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit 8f52792

[IsmaelMartinez]

Cleanup triggers duplicate thread

Real bug, applied in 8c000f12. Wrapped the progress-note cleanup (edit + delete) in its own try/except so that a failure during cleanup — most realistically a GitLab edit_comment exception, which the provider does not catch — no longer falls through to the outer handler and into the "no previous comment found" fallback. That fallback would otherwise publish a fresh suggestions thread even though the persistent comment update at the top of the block had already succeeded, recreating exactly the duplicate-thread bug this PR fixes. Cleanup is now best-effort and never affects the success of the persistent update.

Did not take the optional third part of the suggestion (guarding the fallback block itself). The new try/except already prevents cleanup failures from reaching the fallback; the fallback remains as before for the "persistent edit truly failed" case, which is its original intent. Happy to scope that broader guard into a follow-up PR if maintainers prefer it bundled here.

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit 8c000f1