Skip to content

/improve on GitLab duplicates the persistent suggestions thread on every push once the previous one has any reply #2402

@IsmaelMartinez

Description

@IsmaelMartinez

Summary

On GitLab, pr_code_suggestions.persistent_comment = true is supposed to keep a single ## PR Code Suggestions ✨ discussion that is edited in place across pushes. In practice, once that discussion has any reply (e.g. the author replied to address a suggestion, which makes the thread resolvable and is the only way it can be resolved on GitLab), every subsequent run of /improve leaves the original thread in place and creates a brand-new "Code Suggestions" discussion. The result is one duplicate suggestions thread per push on any MR where the author engages with the bot.

Reproduction

  1. Open an MR. /improve posts discussion A with header ## PR Code Suggestions ✨ and marker <!-- {sha1} -->.
  2. Reply to A (any reply — required for GitLab to allow resolving the thread). Optionally resolve A.
  3. Push another commit so /improve runs again.
  4. Expected: A's comment is edited in place with "Latest suggestions up to {sha2}" + "Previous suggestions up to commit {sha1}".
  5. Actual: a fresh discussion B appears containing exactly that merged body ("Latest suggestions up to {sha2}" + "Previous suggestions up to commit {sha1}"). A stays as-is.

Root cause

In pr_agent/tools/pr_code_suggestions.py:

  • Around pr_code_suggestions.py:107-110, when config.publish_output_progress=true (default) and is_auto_command=false (CLI invocations from CI), pr-agent posts a "Preparing suggestions..." note at the start of the run and stashes it as self.progress_response. On GitLab this immediately creates a fresh discussion (call it B).
  • In publish_persistent_comment_with_history at pr_code_suggestions.py:338-341, when both a previous persistent comment is found AND progress_response is set, the code does:
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

So it edits the progress note B with the merged body and tries to delete the previous persistent comment A.

  • GitLabProvider.remove_comment (pr_agent/git_providers/gitlab_provider.py:761-765) calls comment.delete() inside a bare try/except that just logs. GitLab refuses to delete a note that has replies (HTTP 403), so when the author has interacted with A the delete fails silently. Net effect: A is left intact and B becomes the new persistent comment. On the next push the same thing happens, producing C, D, etc.

Both popular hypotheses are wrong, for reference:

  • It is not a per-commit marker problem. Lookup is by comment.body.startswith(initial_header) at pr_code_suggestions.py:278, where initial_header="## PR Code Suggestions ✨" is passed in from the caller (pr_code_suggestions.py:162) — a stable header.
  • It is not that resolved discussions are filtered out of the listing. GitLabProvider.get_issue_comments (gitlab_provider.py:789-790) calls self.mr.notes.list(get_all=True) which returns notes regardless of resolution state. The original is found — it just gets clobbered by the buggy edit/delete pairing.

Suggested fix

In publish_persistent_comment_with_history, when both a previous persistent comment and a progress_response exist, prefer editing the previously-found comment in place and removing the progress note, rather than the other way around. That keeps the stable thread stable across pushes and limits the silent-delete failure to a throwaway note that is safe to leave behind on error.

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

Trade-off to be aware of

The current ordering is deliberate — the inline comment at line 338 reads # publish to 'progress_response' comment, because it refreshes immediately. The intent is that on GitLab a freshly-posted note renders in the UI more promptly than an edit of an older note. The proposed fix 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.

As a defence-in-depth, GitLabProvider.remove_comment could also detect the "cannot delete a note with replies" case and skip the delete cleanly rather than swallow the 403 — that change is purely additive and worth doing regardless.

A more conservative alternative would be to keep the current "edit the fresh note, delete the old one" path on GitHub (where deletion is reliable) and only flip it on GitLab, but that adds provider-specific branching in a generic helper. The simple swap above seems like a fair trade.

Workaround

Setting config.publish_output_progress = false (e.g. via env CONFIG__PUBLISH_OUTPUT_PROGRESS=false in CI) avoids the problem because progress_response is None and the code falls into the in-place edit path. The trade-off is losing the "Preparing suggestions..." indicator while the run is in flight.

Environment

  • Provider: GitLab (gitlab.com)
  • Auth: GitLab personal access token, is_auto_command=false (CLI invocation from CI)
  • Config: pr_code_suggestions.persistent_comment=true, commitable_code_suggestions=false (also reproduces with true)
  • pr-agent ref: reproduced at 009ba5a116c4d3273368a6dc53a4efdb7904d519, current main at time of filing

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions