fix: propagate exception from verified inline comment publishing

[karesansui-u]

Bug description

In github_provider.py, _publish_inline_comments_fallback_with_verification() has a bare except: pass that silently swallows all exceptions when publishing verified review comments:

if verified_comments:
    try:
        self.pr.create_review(commit=..., comments=verified_comments)
    except:
        pass  # ← review comments silently lost

The caller publish_code_suggestions() believes the operation succeeded and returns True. The one-by-one retry path in pr_code_suggestions.py never activates:

is_successful = self.git_provider.publish_code_suggestions(code_suggestions)
if not is_successful:
    # retry one by one ← never reached

Impact

When the GitHub API returns an error (rate limit, network failure, permission error), review comments are silently dropped. The user sees no output and no error. The retry mechanism designed to handle partial failures is completely bypassed.

Fix

Replace except: pass with except Exception as e: that logs the error and re-raises, allowing the caller to detect the failure and retry.

Affected files

• pr_agent/git_providers/github_provider.py (L478-479) — 2 line change

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

Review Summary by Qodo

Propagate exception from verified inline comment publishing

🐞 Bug fix

Walkthroughs

Description

• Replace bare except: pass with proper exception handling
• Log error details when verified inline comments fail to publish
• Re-raise exception to allow caller to detect failure and retry
• Enables one-by-one retry mechanism in publish_code_suggestions()

Diagram

flowchart LR
  A["publish_code_suggestions()"] -->|calls| B["_publish_inline_comments_fallback_with_verification()"]
  B -->|previously| C["except: pass<br/>silently fails"]
  C -->|result| D["Returns True<br/>no retry"]
  B -->|now| E["except Exception<br/>log and raise"]
  E -->|result| F["Propagates error<br/>enables retry"]

Loading

File Changes

1. pr_agent/git_providers/github_provider.py 🐞 Bug fix +3/-2

Fix exception handling in inline comment publishing

• Replace bare except: pass with except Exception as e: to catch and handle errors
• Add error logging via get_logger().error() with exception details
• Re-raise exception after logging to propagate failure to caller
• Allows publish_code_suggestions() to detect failure and activate one-by-one retry path

pr_agent/git_providers/github_provider.py

---

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

Code Review by Qodo

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

1. Exception message logged via e ☑ 📘 Rule violation ⛨ Security

Description

The new error log interpolates the raw exception message ({e}), which can leak sensitive details
returned by the GitHub API (e.g., request/response fragments). Prefer structured/exception logging
without embedding the exception text directly to reduce secret leakage risk.

Code

pr_agent/git_providers/github_provider.py[479]

+                get_logger().error(f"Failed to publish verified inline comments: {e}")

Evidence

PR Compliance ID 20 requires exception handling that avoids leaking secrets/tokens in logs. The
changed code logs the exception string directly via an f-string (... {e}), which may include
sensitive data depending on the exception content.

pr_agent/git_providers/github_provider.py[478-480]
Best Practice: Learned patterns

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 code logs the raw exception message via an f-string (`{e}`), which may leak sensitive information contained in exception text.
## Issue Context
The handler already re-raises; the main improvement needed is to log safely and preserve context without embedding potentially sensitive exception strings.
## Fix Focus Areas
- pr_agent/git_providers/github_provider.py[478-480]

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

---

2. Low-signal duplicate logging ☑ 🐞 Bug ✓ Correctness

Description

In _publish_inline_comments_fallback_with_verification(), the new exception handler logs only the
exception string (no traceback) and then re-raises, while publish_inline_comments() already logs the
same propagated failure again. This produces duplicate, low-diagnostic error lines when GitHub
review publishing fails, making debugging and alerting noisier and less actionable.

Code

pr_agent/git_providers/github_provider.py[R478-480]

+            except Exception as e:
+                get_logger().error(f"Failed to publish verified inline comments: {e}")
+                raise

Evidence

The new handler logs with get_logger().error(f"... {e}") and re-raises, which records only the
exception message; the caller catch block then logs the same failure again before re-raising.
Elsewhere in the same file, exceptions are logged with get_logger().exception(...), indicating the
intended pattern when traceback is desired.

pr_agent/git_providers/github_provider.py[474-481]
pr_agent/git_providers/github_provider.py[414-430]
pr_agent/git_providers/github_provider.py[462-464]

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 verified-inline-comment publish failure path now logs an error without traceback and then re-raises; the caller also logs again, causing duplicated low-signal error logs.
### Issue Context
- New handler: logs with `get_logger().error(f"... {e}")` and re-raises.
- Upstream handler: catches and logs the same failure again.
- Elsewhere in the file, `get_logger().exception(...)` is used for exception logging with traceback.
### Fix Focus Areas
- pr_agent/git_providers/github_provider.py[474-481]
- pr_agent/git_providers/github_provider.py[414-430]

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

---

ⓘ The new review experience is currently in Beta. Learn more

[karesansui-u]

Updated: removed the duplicate error log. The caller already logs the exception, so this just re-raises to propagate the failure. Keeps it consistent with the file's convention.

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

Persistent review updated to latest commit c534e99

[IsmaelMartinez]

Hi @karesansui-u, I independently hit this exact bug while working on the inline-comment publishing path (my persistent-inline-comments work in PR #2424). When create_review fails, the verified bulk publish swallows the error and review comments are silently dropped with no signal to the user.

I agree with your fix: propagating the exception so publish_code_suggestions returns False lets the existing one-by-one retry in pr_code_suggestions actually run. The one thing I'd add to make it easy to merge is a regression test, so I wrote one on top of your branch — it asserts the failure propagates out of publish_inline_comments and that publish_code_suggestions then returns False so the retry fires. I verified it fails on unpatched main and passes with your fix.

If it's useful, the test commit sits directly on top of your branch so you can cherry-pick it straight in:

• Branch: https://github.com/IsmaelMartinez/pr-agent/tree/fix/bare-except-review-comments-with-test
• Test commit: IsmaelMartinez@88fdd06

[IsmaelMartinez]

Approving. The bare except: pass is still on main, and your diagnosis holds: with the change the failure reaches publish_code_suggestions, which returns False, so the one-by-one retry in pr_code_suggestions finally runs.

Apologies for the wait, both the three months before I first replied and the time since.

Rather than leave the test offer from June hanging, I have pushed it to your branch along with one tidy-up: now the duplicate log is gone the handler only re-raised, so the try went with it. The test goes red against main on both assertions and the suite is green. Nothing left for you to do.

Thanks for this, and for #2256.

[qodo-code-review]

Code Review by Qodo

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

1. _Status422Error docstring is non-imperative 📘 Rule violation ⚙ Maintainability

Description

The added class docstring begins with third-person Mimics instead of an imperative verb. This
violates the required phrasing convention for new docstrings.

Code

tests/unittest/test_github_inline_comment_fallback.py[R9-10]

+    """Mimics a GithubException carrying an HTTP 422 status, which triggers the
+    verification fallback in ``publish_inline_comments``."""

Relevance

● Weak

Recent rejection explicitly declined imperative-phrasing feedback for a new test docstring under the
same rule.

PR-#2661

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 2694688 requires imperative phrasing, while the newly added docstring starts with the
descriptive third-person verb Mimics.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_github_inline_comment_fallback.py[9-10]

Agent prompt

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

## Issue description
Rewrite the `_Status422Error` docstring so its first sentence uses imperative phrasing, such as `Mimic ...`, rather than third-person `Mimics ...`.

## Issue Context
PR Compliance ID 2694688 requires newly added docstrings to use imperative phrasing.

## Fix Focus Areas
- tests/unittest/test_github_inline_comment_fallback.py[9-10]

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

---

2. Regression docstring lacks imperative 📘 Rule violation ⚙ Maintainability

Description

The new test docstring starts with the noun phrase Regression for #2261 rather than an imperative
description. It therefore does not meet the required docstring phrasing convention.

Code

tests/unittest/test_github_inline_comment_fallback.py[R25-27]

+    """Regression for #2261: when the fallback bulk-publishes the verified
+    comments and that GitHub call fails (rate limit / network / 5xx), the error
+    must propagate instead of being silently swallowed."""

Relevance

● Weak

Recent precedent rejected an equivalent request to rewrite a descriptive test docstring into
imperative phrasing.

PR-#2661

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 2694688 requires an imperative first sentence, but this added function docstring begins with
the non-imperative noun phrase Regression for #2261.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_github_inline_comment_fallback.py[25-27]

Agent prompt

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

## Issue description
Rewrite the regression test docstring to start with an imperative verb, for example `Verify that ...`.

## Issue Context
PR Compliance ID 2694688 requires newly added function docstrings to begin with imperative phrasing.

## Fix Focus Areas
- tests/unittest/test_github_inline_comment_fallback.py[25-27]

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

---

3. Contract docstring lacks imperative 📘 Rule violation ⚙ Maintainability

Description

The second test docstring begins with the descriptive phrase The contract the bug breaks instead
of an imperative verb. This violates the convention for newly added function docstrings.

Code

tests/unittest/test_github_inline_comment_fallback.py[R40-42]

+    """The contract the bug breaks: publish_code_suggestions must return False
+    when comments were not actually published, so the one-by-one retry in
+    pr_code_suggestions runs instead of reporting success."""

Relevance

● Weak

Recent precedent rejected an equivalent imperative-phrasing request for a descriptive test
docstring.

PR-#2661

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 2694688 mandates imperative phrasing, whereas the added docstring opens with the descriptive
noun phrase The contract the bug breaks.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_github_inline_comment_fallback.py[40-42]

Agent prompt

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

## Issue description
Rewrite the test docstring to begin with an imperative verb, such as `Verify that publish_code_suggestions ...`.

## Issue Context
PR Compliance ID 2694688 requires newly added function docstrings to use imperative phrasing.

## Fix Focus Areas
- tests/unittest/test_github_inline_comment_fallback.py[40-42]

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

---

View low (1)

4. Setup comments lack imperatives 📘 Rule violation ⚙ Maintainability

Description

The added setup comments narrate mock-call behavior with arrow fragments and All comments verify
rather than phrasing the setup as instructions. These behavior-describing comments violate the
imperative-comment requirement.

Code

tests/unittest/test_github_inline_comment_fallback.py[R29-32]

+    # 1st create_review (initial bulk) -> 422 to enter the fallback path
+    # 2nd create_review (verified bulk inside fallback) -> transient failure
+    provider = _make_provider([_Status422Error("invalid"), RuntimeError("rate limited")])
+    # All comments verify as valid; avoids the real verification API + sleep(1).

Relevance

● Weak

Same-day precedent rejected imperative rewrites for narrative behavioral comments in unit-test setup
code.

PR-#2785

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 2694688 requires behavior comments to use imperative instructions; the added comments instead
use descriptive arrow notation and the narrative phrase All comments verify as valid.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_github_inline_comment_fallback.py[29-32]

Agent prompt

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

## Issue description
Rewrite the setup comments with imperative phrasing, such as `Trigger the fallback ...`, `Raise a transient failure ...`, and `Stub comment verification ...`.

## Issue Context
PR Compliance ID 2694688 requires comments that describe code behavior to be phrased as commands or instructions.

## Fix Focus Areas
- tests/unittest/test_github_inline_comment_fallback.py[29-32]

ⓘ 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: ⚖️ Balanced

Tip of the day

💡 Did you know, you can start a comment with 'qodo' or '@qodo' to chat about any finding

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗