fix(review): remove stale progress comments

[oleksii-tumanov]

Summary

• Remove the exact /review progress comment when quiet output suppresses the review or review generation fails.
• Leave unrelated temporary comments untouched when the current run has no progress handle.
• Normalize Bitbucket Cloud comment handles for targeted deletion.

Related to #2648, which applied exact-handle cleanup to /improve.

Result

When /review posts Preparing review..., quiet no-findings exits and errors no longer leave that placeholder behind. Runs that do not publish a progress comment do not delete existing comments.

Testing

• PYTHONPATH=. pytest tests/unittest/test_pr_reviewer_core.py tests/unittest/test_bitbucket_provider.py -q (73 passed)
• PYTHONPATH=. pytest tests/unittest -q (2,039 passed, 1 skipped, 1 expected failure)
• git diff --check (passed)

[qodo-code-review]

PR Summary by Qodo

Fix /review: reliably delete stale “Preparing review…” progress comments

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Track the exact /review progress comment handle and delete it on all exits.
• Avoid deleting unrelated temporary comments when no progress comment was posted.
• Normalize Bitbucket Cloud comment handles so targeted deletion always works.

Diagram

graph TD
  A["/review command"] --> B["PRReviewer.run()"] --> C["GitProvider.publish_comment(temp)"] --> D["Prepare prediction"] --> E["Publish final review (optional)"] --> F["GitProvider.remove_comment(handle)"] --> G{{"Bitbucket comments API"}}

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Introduce a ProgressComment context manager/helper

• ➕ Encapsulates publish+cleanup in one reusable abstraction (usable by /improve, /review, etc.)
• ➕ Reduces risk of future tools forgetting to clean up on early return/exception
• ➖ Slightly more code/indirection for a small, currently localized behavior
• ➖ May require broader refactors across tools/providers to be worthwhile

2. Standardize provider return type for publish_comment

• ➕ Avoids needing provider-specific normalization in remove_comment (always return numeric id)
• ➕ Makes tool code simpler and more consistent across providers
• ➖ Potentially breaking change if other code relies on provider-specific response objects
• ➖ Would require coordinated updates across multiple providers and tests

Recommendation: The PR’s approach is appropriate for the scope: capture the returned progress handle and delete it in a finally block, ensuring cleanup on quiet exits and failures without touching unrelated comments. Consider a small helper/context manager later if multiple tools need identical progress-comment lifecycle handling.

Files changed (4) +151 / -5

Bug fix (2) +10 / -5

bitbucket_provider.pyAllow comment deletion by dict handle or numeric id (Bitbucket Cloud) +2/-1

Allow comment deletion by dict handle or numeric id (Bitbucket Cloud)

• Updates remove_comment to accept either a stored numeric id or a provider-returned comment dict and delete using the normalized comment id. This enables targeted deletion even when publish_comment returns a structured response.

pr_agent/git_providers/bitbucket_provider.py

pr_reviewer.pyTrack and always clean up the /review progress comment +8/-4

Track and always clean up the /review progress comment

• Stores the handle returned by publish_comment when posting “Preparing review…”. Moves cleanup into a finally block that deletes only that specific progress comment, preventing stale placeholders on quiet no-findings exits or review-generation failures and avoiding deletion when no progress comment was posted.

pr_agent/tools/pr_reviewer.py

Tests (2) +141 / -0

test_bitbucket_provider.pyTest BitbucketProvider.remove_comment supports dict or id inputs +9/-0

Test BitbucketProvider.remove_comment supports dict or id inputs

• Adds a parametrized unit test verifying remove_comment deletes the correct endpoint whether given a dict with an id or a raw numeric id.

tests/unittest/test_bitbucket_provider.py

test_pr_reviewer_core.pyAdd reviewer lifecycle tests for progress-comment cleanup paths +132/-0

Add reviewer lifecycle tests for progress-comment cleanup paths

• Adds async tests covering cleanup when quiet output suppresses the review, when review generation fails (with/without propagated errors), and when no progress comment is published. Updates the existing threading test to assert the progress comment is removed via remove_comment rather than remove_initial_comment.

tests/unittest/test_pr_reviewer_core.py

[qodo-code-review]

Code Review by Qodo

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

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

• Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

Tip of the day

💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[IsmaelMartinez]

Thanks for carrying #2648's fix through to /review, and for going with a targeted handle instead of a blanket sweep. Three notes inline, no blockers.

[IsmaelMartinez]

Right shape, and I prefer it to the /improve version it follows, which repeats remove_comment at each exit and so has to be rechecked every time an early return is added.

One thing worth confirming as deliberate: a failed /review now leaves nothing on the PR at all. /improve posts a "Failed to generate code suggestions" notice only on the branch where there was no progress comment to remove, so it does not really settle it either way.

[oleksii-tumanov]

I agree that a visible failure result could be better for manual runs.
I kept this change smaller and focused, but the additional fix does not look too large. I can add it to this PR with the relevant cross-provider behavior and tests if it fits project approach, or handle it in a follow-up PR.
What do you suggest?

[IsmaelMartinez]

Checked what every provider returns here against its remove_comment: GitHub and GitLab hand back a comment object that deletes, Azure a Comment carrying thread_id, Gitea a dict its remove_comment already unpacks, and Bitbucket Server, Gerrit, CodeCommit and local all return None for a temporary comment, so the new finally is a no-op there. Bitbucket Cloud was the only one that needed the fix.

[oleksii-tumanov]

That matches what I had in mind:

• keep the cleanup generic
• skip it when a provider returns None
• handle the Bitbucket Cloud difference inside its provider.

In the future, we could standardize the comment handle across providers, but that feels separate from this PR.

[IsmaelMartinez]

Both shapes are genuinely live, so this needs to stay tolerant: remove_initial_comment still feeds it the bare id out of temp_comments, while the new caller passes the whole dict.

[oleksii-tumanov]

Yep, I kept it compatible with both forms. We could normalize the handle later, but I think changing the provider boundary belongs in a separate PR.

[IsmaelMartinez]

Approving, and keep the failure result separate.

Open it as a follow-up: it changes behaviour on every provider, so it wants its own test matrix, and bundling it would put a small safe fix behind a larger argument. Your instinct on the provider boundary is the same one, and I agree with it.