fix(review): publish failure result for manual runs

[oleksii-tumanov]

Summary

• publish one generic failure result when a manual /review run raises
• remove the exact progress comment first while keeping automatic and non-publishing runs silent
• treat an optional persistent-update notice as best-effort after the review was edited, while preserving pre-edit fallback behavior in the shared path and Bitbucket Cloud
• preserve original error propagation and contain cleanup or reporting failures

Follow-up to #2788.

Result

A failed manual /review now leaves Failed to review PR instead of leaving no visible outcome. The underlying exception remains in logs and is not exposed in the public comment.

If an existing persistent review was already updated successfully, a failure posting only the latest-commit notice no longer produces a misleading failure result or a duplicate full review.

Testing

• PYTHONPATH=. pytest -q tests/unittest/test_pr_reviewer_core.py tests/unittest/test_gitlab_provider.py tests/unittest/test_bitbucket_provider.py (199 passed)
• PYTHONPATH=. pytest -q tests/unittest --ignore=tests/unittest/test_extra_config_url.py (2,059 passed, 1 skipped, 1 xfailed)
• PYTHONPATH=. pytest -q tests/unittest/test_extra_config_url.py (41 passed)

[qodo-code-review]

PR Summary by Qodo

Publish generic failure comment for manual /review runs

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Publish a generic failure comment when a manual /review run raises.
• Always remove the temporary progress comment before publishing failure.
• Keep auto-command and non-publishing runs silent; preserve original exception behavior.

Diagram

graph TD
  U([Manual "/review"]) --> R["PRReviewer.run()"] --> D{Error raised?} -->|No| P["Publish review"] --> E([Done])
  D -->|Yes| C["Cleanup progress"] --> G{Manual + publish_output?} -->|Yes| F["Publish failure comment"] --> E
  G -->|No| E
  subgraph Legend
    direction LR
    _start([Start/End]) ~~~ _proc["Process"] ~~~ _dec{Decision}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Edit the progress comment into a failure message

• ➕ Avoids creating an extra comment in the thread
• ➕ Keeps outcome tightly linked to the original temporary status
• ➖ Requires a reliable comment handle (not always available)
• ➖ Some providers may not support edits uniformly; increases provider-specific complexity

2. Publish a GitHub/SCM status check instead of a comment

• ➕ Separates user-facing discussion from machine status
• ➕ Better fits CI-style pass/fail semantics and can be aggregated in checks UI
• ➖ Requires additional integration surface (checks API) across providers
• ➖ Less visible for users expecting an in-thread response to /review

3. Include a short opaque error reference in the comment

• ➕ Helps correlate user-visible failures with logs without exposing details
• ➕ Can reduce support/debugging time
• ➖ Adds user-facing complexity and potential information leakage risk
• ➖ Needs consistent correlation-id generation and logging conventions

Recommendation: The PR’s approach (publish a generic failure comment only for manual runs when output is enabled) is a good balance of UX and security: it provides a visible outcome without exposing internal error details. Editing the progress comment could reduce noise but is less robust when no progress handle exists, which this PR explicitly accounts for.

Files changed (2) +210 / -5

Bug fix (1) +8 / -0

pr_reviewer.pyPublish generic failure result for manual runs on exceptions +8/-0

Publish generic failure result for manual runs on exceptions

• Tracks whether the review run failed and, during finalization, publishes a generic "Failed to review PR" comment for manual runs when output publishing is enabled. Progress comment cleanup remains best-effort and both cleanup/publication failures are contained without masking the original review error behavior.

pr_agent/tools/pr_reviewer.py

Tests (1) +202 / -5

test_pr_reviewer_core.pyAdd coverage for failure-result publication and error propagation +202/-5

Add coverage for failure-result publication and error propagation

• Updates the existing progress-cleanup failure test to assert both comment sequence and exception identity. Adds new async tests covering: failure when progress comment has no handle, failure before progress is posted, suppression for auto/disabled output modes, cleanup failure resilience, and ensuring failure-comment publication does not mask the underlying error.

tests/unittest/test_pr_reviewer_core.py

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Single quotes in test_gitlab_provider ✓ Resolved 📘 Rule violation ⚙ Maintainability ⭐ New

Description

The new GitLab provider unit tests introduce single-quoted string literals (e.g., ['body'],
{'notes': ...}), violating the project requirement to use double quotes for Python string
literals. This reduces consistency with the enforced style and can cause lint failures.

Code

tests/unittest/test_gitlab_provider.py[506]

+        assert "updated to latest commit" in gitlab_provider.mr.notes.create.call_args.args[0]['body']

Relevance

●●● Strong

Recent accepted test-style precedent explicitly requires replacing single-quoted literals with
double quotes.

PR-#2569
PR-#2526

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2694657 requires double quotes for Python string literals. The newly added test
code uses single-quoted literals such as ['body'] and dict keys like {'notes': ...}.

Rule 2694657: Use double quotes for all Python string literals
tests/unittest/test_gitlab_provider.py[506-528]

Agent prompt

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

## Issue description
The updated tests in `tests/unittest/test_gitlab_provider.py` use single-quoted Python string literals (e.g., `['body']`, `{'notes': ...}`), but the compliance checklist requires double quotes for Python strings.

## Issue Context
This violates PR Compliance ID 2694657 and can lead to style/lint inconsistencies.

## Fix Focus Areas
- tests/unittest/test_gitlab_provider.py[506-528]

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

---

2. Missing traceback in warning ✓ Resolved 🐞 Bug ◔ Observability ⭐ New

Description

The persistent-update notification failure path in the new generic provider implementation (and its
Bitbucket usage) logs only str(e) at warning level and then returns, which drops the stack trace
needed to debug intermittent failures when posting the optional “updated to latest commit” status
message. This reduces observability and makes production incidents harder to root-cause across
providers that rely on this base behavior.

Code

pr_agent/git_providers/git_provider.py[R396-400]

+                        except Exception as e:
+                            # The review was already updated in place; a notification failure must not reach
+                            # the outer except, whose fallback publish would duplicate the review.
+                            get_logger().warning(f"Failed to publish persistent review update message: {e}")
+                            return comment

Relevance

●●● Strong

Recent accepted precedents favor preserving traceback and improving diagnostics in exception logging
paths.

PR-#2424

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

In GitProvider.publish_persistent_comment_full() a new inner try/except around publishing the
optional update/status message logs a warning containing only the exception message, while an
existing outer exception handler in the same flow uses get_logger().exception(...) to record full
stack traces. Because the inner path does not include exc_info (or otherwise log the exception),
it loses the traceback that would otherwise be available for diagnosing API/client failures, even
though the error is intentionally swallowed to avoid triggering the outer fallback that could
duplicate the review.

pr_agent/git_providers/git_provider.py[393-400]
pr_agent/git_providers/git_provider.py[402-403]
pr_agent/git_providers/bitbucket_provider.py[414-420]
pr_agent/git_providers/bitbucket_provider.py[422-423]

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_full()`/`publish_persistent_comment()` intentionally swallow exceptions when publishing the optional “updated to latest commit” status message to avoid triggering the outer fallback (which could duplicate the review), but the current logging uses `warning(f"... {e}")` and drops the traceback.

## Issue Context
There is already an outer exception handler that uses `get_logger().exception(...)` and therefore preserves stack traces; the new inner failure path should provide comparable debugging signal (e.g., `exc_info=True` or equivalent) while still swallowing the exception and returning the existing `comment`.

## Fix Focus Areas
- pr_agent/git_providers/git_provider.py[393-400]
- pr_agent/git_providers/bitbucket_provider.py[414-420]

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

---

3. Misleading failure after persistent update ✓ Resolved 🐞 Bug ≡ Correctness

Description

PRReviewer publishes "Failed to review PR" for any exception, including failures that can happen
after a persistent review comment was already successfully edited (e.g., failing to post the
optional final update message). This can leave a correct updated review in-place while also posting
a failure result, misleading users about the actual outcome.

Code

pr_agent/tools/pr_reviewer.py[R222-225]

+            if (review_failed and get_settings().config.publish_output and
+                    not get_settings().config.get('is_auto_command', False)):
+                try:
+                    self.git_provider.publish_comment("Failed to review PR")

Relevance

●●● Strong

Recent PR #2404 accepted isolating post-persistence cleanup failures to prevent misleading fallback
outcomes.

PR-#2404

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR change posts a failure comment in finally whenever review_failed is set by the broad
exception handler. Separately, persistent-comment updates can succeed (edit the existing comment)
and then raise when attempting to publish the optional final update message; since that exception is
not isolated, it will trip the new failure-result logic and produce an incorrect failure comment
despite the review being updated.

pr_agent/tools/pr_reviewer.py[189-227]
pr_agent/git_providers/git_provider.py[364-399]
pr_agent/git_providers/github_provider.py[478-499]
PR-#2404

Agent prompt

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

### Issue description
`PRReviewer.run()` now posts a generic failure comment whenever any exception bubbles out of the main try-block. However, a “persistent review update” can already be successfully applied (comment edited in-place) and then fail later when posting the optional `final_update_message`. That late, non-critical failure currently throws, causing `review_failed=True` and triggering a misleading `"Failed to review PR"` comment.

### Issue Context
`GitProvider.publish_persistent_comment_full()` edits the existing persistent comment, then (optionally) posts a separate “updated to latest commit” comment. That extra publish is not protected by a try/except and can raise. With this PR’s new failure-result publication, that exception will create an incorrect public failure signal even though the persistent review itself is already updated.

### Fix Focus Areas
- pr_agent/git_providers/git_provider.py[370-395]
- pr_agent/tools/pr_reviewer.py[211-227]

### Suggested change
Wrap the `final_update_message` publication in `publish_persistent_comment_full()` in its own try/except so it cannot raise after a successful edit. On failure, log and return the updated `comment` (or otherwise treat the update-message as best-effort). This prevents the new reviewer failure-result comment from being posted in “partial success” cases.

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

---

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

Context sources

✅ Compliance rules (platform): 34 rules

Review mode: 🚀 Fast: The latest push makes localized exception-logging changes in two provider paths plus test updates; behavior and blast radius are contained to post-update notification failures.

Tip of the day

💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 3c7c3be

Results up to commit 0e2de69 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 📜 Skill insights (0)

1. Misleading failure after persistent update ✓ Resolved 🐞 Bug ≡ Correctness

Description

PRReviewer publishes "Failed to review PR" for any exception, including failures that can happen
after a persistent review comment was already successfully edited (e.g., failing to post the
optional final update message). This can leave a correct updated review in-place while also posting
a failure result, misleading users about the actual outcome.

Code

pr_agent/tools/pr_reviewer.py[R222-225]

+            if (review_failed and get_settings().config.publish_output and
+                    not get_settings().config.get('is_auto_command', False)):
+                try:
+                    self.git_provider.publish_comment("Failed to review PR")

Relevance

●●● Strong

Recent PR #2404 accepted isolating post-persistence cleanup failures to prevent misleading fallback
outcomes.

PR-#2404

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR change posts a failure comment in finally whenever review_failed is set by the broad
exception handler. Separately, persistent-comment updates can succeed (edit the existing comment)
and then raise when attempting to publish the optional final update message; since that exception is
not isolated, it will trip the new failure-result logic and produce an incorrect failure comment
despite the review being updated.

pr_agent/tools/pr_reviewer.py[189-227]
pr_agent/git_providers/git_provider.py[364-399]
pr_agent/git_providers/github_provider.py[478-499]
PR-#2404

Agent prompt

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

### Issue description
`PRReviewer.run()` now posts a generic failure comment whenever any exception bubbles out of the main try-block. However, a “persistent review update” can already be successfully applied (comment edited in-place) and then fail later when posting the optional `final_update_message`. That late, non-critical failure currently throws, causing `review_failed=True` and triggering a misleading `"Failed to review PR"` comment.

### Issue Context
`GitProvider.publish_persistent_comment_full()` edits the existing persistent comment, then (optionally) posts a separate “updated to latest commit” comment. That extra publish is not protected by a try/except and can raise. With this PR’s new failure-result publication, that exception will create an incorrect public failure signal even though the persistent review itself is already updated.

### Fix Focus Areas
- pr_agent/git_providers/git_provider.py[370-395]
- pr_agent/tools/pr_reviewer.py[211-227]

### Suggested change
Wrap the `final_update_message` publication in `publish_persistent_comment_full()` in its own try/except so it cannot raise after a successful edit. On failure, log and return the updated `comment` (or otherwise treat the update-message as best-effort). This prevents the new reviewer failure-result comment from being posted in “partial success” cases.

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

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit df6cc50

[qodo-code-review]

Code review by qodo was updated up to the latest commit 6c534d1

[qodo-code-review]

No code changes since the last review — review skipped

[IsmaelMartinez]

Thanks for the fast turnaround on the Qodo findings, each fixed within minutes of being raised. Verified against today's main: with your three source files reverted and the tests kept, nine go red across reviewer core, GitLab and Bitbucket, so the coverage is real, and the failure comment correctly stays out of automatic runs. #2532 landed this morning and touched the same test file, so I resolved the test-only conflict on the branch to save you the round trip; the full suite is green on the merged tree. Merging, and #2797 can rebase over this.