Skip to content

fix: propagate exception from verified inline comment publishing#2258

Open
karesansui-u wants to merge 2 commits intoqodo-ai:mainfrom
karesansui-u:fix/bare-except-review-comments
Open

fix: propagate exception from verified inline comment publishing#2258
karesansui-u wants to merge 2 commits intoqodo-ai:mainfrom
karesansui-u:fix/bare-except-review-comments

Conversation

@karesansui-u
Copy link
Copy Markdown

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

The bare except:pass in _publish_inline_comments_fallback_with_verification
silently swallows all exceptions when publishing verified review comments.
This causes publish_code_suggestions to believe the operation succeeded,
preventing the one-by-one retry path from activating.

Replace with Exception logging and re-raise so the caller can detect the
failure and retry individual comments.
@qodo-free-for-open-source-projects
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Propagate exception from verified inline comment publishing

🐞 Bug fix

Grey Divider

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

Grey Divider

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


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown
Contributor

qodo-free-for-open-source-projects bot commented Mar 16, 2026

Code Review by Qodo

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

Grey Divider


Remediation recommended

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&amp;quot;... {e}&amp;quot;)` 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


Grey Divider

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

Grey Divider

Qodo Logo

Address review feedback: the caller already logs the exception, so
logging here causes duplicate entries. Just re-raise and let the
upstream handler log with full context.
@karesansui-u
Copy link
Copy Markdown
Author

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
Copy link
Copy Markdown
Contributor

qodo-free-for-open-source-projects bot commented Mar 16, 2026

Persistent review updated to latest commit c534e99

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant