Skip to content

Fix: log dropped inline comments - #2876

Merged
IsmaelMartinez merged 3 commits into
The-PR-Agent:mainfrom
raappo:fix/issue-2874-log-dropped-inline-comments
Aug 29, 2026
Merged

Fix: log dropped inline comments#2876
IsmaelMartinez merged 3 commits into
The-PR-Agent:mainfrom
raappo:fix/issue-2874-log-dropped-inline-comments

Conversation

@raappo

@raappo raappo commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Fixes #2874\n\nLogs paths of invalid inline comments that are dropped because they could not be converted to a one-line comment or because try_fix_invalid_inline_comments is off.

Copilot AI lite review requested due to automatic review settings August 28, 2026 18:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Log invalid inline comments dropped by GitHub fallback

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Warns when invalid inline comments cannot be repaired or repair is disabled.
• Includes dropped file paths and counts for actionable diagnostics.
• Adds regression coverage for both comment-loss scenarios.
Diagram

graph TD
  A["Inline comments"] --> B["GitHub verification"] --> C{"Comment valid"}
  C -->|Yes| D["Publish review"]
  C -->|No| E{"Repair enabled"}
  E -->|Yes| F["One-line repair"] --> D
  E -->|No| G["Drop warning"]
  F -->|Unfixable| G
Loading
High-Level Assessment

Logging at the existing fallback decision points is the most direct approach because those branches know both why comments were dropped and which paths were affected. A broader result-reporting abstraction would add unnecessary complexity for this focused observability fix.

Files changed (2) +65 / -2

Bug fix (1) +13 / -2
github_provider.pyWarn when GitHub drops invalid inline comments +13/-2

Warn when GitHub drops invalid inline comments

• Tracks invalid comments that cannot be converted to one-line comments and logs their count and file paths. Also warns with all affected paths when automatic repair is disabled.

pr_agent/git_providers/github_provider.py

Tests (1) +52 / -0
test_github_provider_fallback_verification.pyCover dropped-comment warning paths +52/-0

Cover dropped-comment warning paths

• Adds unit tests asserting path-aware warnings when repair only succeeds partially and when try_fix_invalid_inline_comments is disabled.

tests/unittest/test_github_provider_fallback_verification.py

@qodo-code-review

qodo-code-review Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

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

Grey Divider


Remediation recommended

1. Unrelated file-wide reformatting 📘 Rule violation ⚙ Maintainability ⭐ New
Description
Most changes to github_provider.py are cosmetic reformatting far outside the dropped-comment
logging implementation. This violates the requirement to keep non-functional edits localized and
makes the behavioral change harder to review safely.
Code

pr_agent/git_providers/github_provider.py[R1641-1643]

+                            if (
+                                min_distance < 10
+                            ):  # 10 lines - a reasonable distance to consider the comment inside the hunk
Relevance

●● Moderate

Accepted formatting feedback is common, but a close rejection of cosmetic formatting creates mixed
evidence for wholesale reformatting.

PR-#2424
PR-#2796

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 2694684 prohibits wholesale reformatting unrelated to the intended behavior. The
cited hunk only wraps an existing condition and reformats existing dictionary accesses, while the
actual dropped-comment logging is localized elsewhere at
pr_agent/git_providers/github_provider.py[836-851]; similar cosmetic edits span the file.

Rule 2694684: Limit non-functional changes in modified files
pr_agent/git_providers/github_provider.py[1641-1651]
pr_agent/git_providers/github_provider.py[836-851]
pr_agent/git_providers/github_provider.py[1677-1684]

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 PR includes file-wide cosmetic changes unrelated to logging dropped inline comments.

## Issue Context
Keep the functional logging changes and their tests, but restore unrelated code to its prior formatting so the diff remains localized and reviewable.

## Fix Focus Areas
- pr_agent/git_providers/github_provider.py[1641-1651]
- pr_agent/git_providers/github_provider.py[1677-1684]

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


2. Warning call exceeds 120 characters ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The added get_logger().warning(...) statement is longer than the configured 120-character maximum.
Additional new test assertions and mock setup lines also exceed that limit.
Code

pr_agent/git_providers/github_provider.py[793]

+                get_logger().warning(f"Dropped {dropped_count} invalid comments that could not be fixed. Paths: {dropped_paths}")
Relevance

●●● Strong

Recent same-file precedent accepted wrapping newly added code to satisfy Ruff’s 120-character limit.

PR-#2776

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rules 2694655 and 2694690 both set a 120-character maximum, matching pyproject.toml's Ruff
configuration. The added warning call at line 793 and several added test lines exceed that bound.

Rule 2694655: Enforce 120-character maximum line length in Python source per Ruff config
Rule 2694690: Enforce maximum line length of 120 characters
pr_agent/git_providers/github_provider.py[793-793]
tests/unittest/test_github_provider_fallback_verification.py[16-16]

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

## Issue description
New Python source lines exceed the 120-character maximum configured for Ruff.

## Issue Context
Wrap the logger calls, mock setup, context managers, and assertions using parentheses while preserving behavior.

## Fix Focus Areas
- pr_agent/git_providers/github_provider.py[793-796]
- tests/unittest/test_github_provider_fallback_verification.py[16-29]
- tests/unittest/test_github_provider_fallback_verification.py[42-51]

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


3. Added lines contain trailing whitespace ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
New blank and comprehension lines contain spaces immediately before their newline characters. This
introduces whitespace-check failures in both modified Python files.
Code

pr_agent/git_providers/github_provider.py[786]

+            
Relevance

●●● Strong

Recent same-file precedent accepted removing whitespace-only and trailing-space lines.

PR-#2424

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694658 prohibits spaces or tabs immediately before a line break. The added whitespace-only
line 786 and line 790's trailing space demonstrate the violation, with further whitespace-only
additions in the test file.

Rule 2694658: Disallow trailing whitespace in source files
pr_agent/git_providers/github_provider.py[786-790]
tests/unittest/test_github_provider_fallback_verification.py[17-17]

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

## Issue description
Several added lines contain trailing spaces.

## Issue Context
Strip spaces and tabs occurring after the final visible character, including whitespace-only blank lines in the production and test files.

## Fix Focus Areas
- pr_agent/git_providers/github_provider.py[786-790]
- tests/unittest/test_github_provider_fallback_verification.py[17-27]
- tests/unittest/test_github_provider_fallback_verification.py[43-49]

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


View medium (4)
4. Test file has extra newline ✓ Resolved 📘 Rule violation ✧ Quality
Description
The newly added test ends with an additional blank line after its last statement instead of exactly
one trailing newline. This violates the source-file EOF requirement.
Code

tests/unittest/test_github_provider_fallback_verification.py[52]

+
Relevance

●●● Strong

This is a trivial deterministic EOF formatting fix, consistent with the repository’s accepted
whitespace hygiene enforcement.

PR-#2424

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694661 requires exactly one trailing newline and no extra newline-only line. The diff adds
blank line 52 after the final assertion on line 51.

Rule 2694661: Source files must end with a single trailing newline character
tests/unittest/test_github_provider_fallback_verification.py[51-52]

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 new test file has an extra newline-only line at EOF.

## Issue Context
End the file immediately after line 51 with one newline character and no additional blank line.

## Fix Focus Areas
- tests/unittest/test_github_provider_fallback_verification.py[51-52]

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


5. test imports are misgrouped ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The new test places third-party and project imports before the standard-library unittest.mock
import, with no blank lines between sections. This violates the required isort section order.
Code

tests/unittest/test_github_provider_fallback_verification.py[R1-4]

+import pytest
+from pr_agent.git_providers.github_provider import GithubProvider
+from pr_agent.config_loader import get_settings
+from unittest.mock import MagicMock, patch
Relevance

●●● Strong

Recent test-file precedents accepted restoring standard-library-first and blank-line-separated
import grouping.

PR-#2637
PR-#2796

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694656 requires standard-library imports first, followed by third-party and local imports with
blank-line separation. The added block starts with pytest, then pr_agent, and places
unittest.mock last.

Rule 2694656: Group Python imports according to isort sections and order
tests/unittest/test_github_provider_fallback_verification.py[1-4]

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 new test import block does not follow the required standard-library, third-party, then local-project order.

## Issue Context
Place `unittest.mock` first, `pytest` in the third-party section, and `pr_agent` imports in the local section, with blank lines between sections.

## Fix Focus Areas
- tests/unittest/test_github_provider_fallback_verification.py[1-4]

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


6. patch paths use single quotes ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The new Python test uses single-quoted module path literals in both patch() calls. The checklist
requires double quotes for Python string literals when doing so does not require extra escaping.
Code

tests/unittest/test_github_provider_fallback_verification.py[R22-23]

+    with patch('pr_agent.git_providers.github_provider.get_logger') as mock_logger, \
+         patch('pr_agent.git_providers.github_provider.get_settings') as mock_settings:
Relevance

●●● Strong

Recent same-file precedent accepted converting newly added single-quoted strings to double quotes.

PR-#2796

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694657 requires double quotes for Python string literals except where escaping justifies
single quotes; these module paths contain no double quotes and need no such exception.

Rule 2694657: Use double quotes for all Python string literals
tests/unittest/test_github_provider_fallback_verification.py[22-23]
tests/unittest/test_github_provider_fallback_verification.py[44-45]

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 added `patch()` module paths use single-quoted Python string literals.

## Issue Context
Replace these delimiters with double quotes without changing the path values, and make the same correction in the second test.

## Fix Focus Areas
- tests/unittest/test_github_provider_fallback_verification.py[22-23]
- tests/unittest/test_github_provider_fallback_verification.py[44-45]

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


7. Failed conversions omit paths ✓ Resolved 🐞 Bug ◔ Observability
Description
dropped_paths infers conversion success from the original comment's fields instead of the fixer's
actual result, so a comment with start_line, start_side, or a suggestion is excluded even when
_try_fix_invalid_inline_comments catches an exception and drops it. The warning then reports a
positive dropped count with an incomplete or empty path list, defeating the purpose of this change
for failed conversions.
Code

pr_agent/git_providers/github_provider.py[R789-792]

+                dropped_paths = [
+                    c.get("path") for c, _ in invalid_comments 
+                    if not ("```suggestion" in c.get("body", "") or "start_line" in c or "start_side" in c)
+                ]
Relevance

●● Moderate

Specific correctness concern lacks a closely matching accepted or rejected precedent; semantic
behavior requires review.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The fixer catches any per-comment conversion exception and omits that comment from its return list,
while the new path filter excludes originals solely because they contain fields that normally
trigger modification. For example, an invalid comment with start_line but no body raises at
comment["body"], is counted as dropped, but its path is filtered out because start_line is
present.

pr_agent/git_providers/github_provider.py[787-793]
pr_agent/git_providers/github_provider.py[840-854]
tests/unittest/test_github_provider_fallback_verification.py[12-29]

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 dropped-path warning reconstructs failures from comment shape, but the fixer can catch an exception and return no fixed comment for a shape that the filter assumes was fixed. Track which original comments were successfully converted so every unsuccessful conversion contributes its path.

## Issue Context
`_try_fix_invalid_inline_comments` currently returns only fixed dictionaries, so the caller cannot reliably associate successes and failures with the original comments. Adjust its result or perform per-comment conversion with explicit success tracking; keep publication failures distinct from conversion failures.

## Fix Focus Areas
- pr_agent/git_providers/github_provider.py[778-793]
- pr_agent/git_providers/github_provider.py[832-855]
- tests/unittest/test_github_provider_fallback_verification.py[6-29]

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



Informational

8. Comments use narrative phrasing ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The new comments Should have warned ... describe expected behavior narratively rather than giving
an imperative instruction. Both test cases introduce this phrasing.
Code

tests/unittest/test_github_provider_fallback_verification.py[28]

+        # Should have warned about 1 dropped comment
Relevance

● Weak

Recent precedent rejected rewriting narrative behavioral comments into imperative phrasing.

PR-#2774
PR-#2785

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694688 requires behavior-describing comments to use imperative phrasing. `Should have warned
about 1 dropped comment` and its second-test counterpart are descriptive rather than imperative.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_github_provider_fallback_verification.py[28-28]
tests/unittest/test_github_provider_fallback_verification.py[50-50]

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

## Issue description
Two added comments use descriptive narrative phrasing rather than imperative phrasing.

## Issue Context
Rewrite the comments as concise instructions, such as `# Verify one dropped-comment warning.` and `# Verify two dropped-comment warnings.`

## Fix Focus Areas
- tests/unittest/test_github_provider_fallback_verification.py[28-28]
- tests/unittest/test_github_provider_fallback_verification.py[50-50]

ⓘ 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.

Grey Divider

Context sources
✅ Compliance rules (platform): 34 rules
Review mode: ⚖️ Balanced

Grey Divider

Tip of the day
💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit 6cc4ec0

Results up to commit 77089fb ⚖️ Balanced


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


Remediation recommended
1. Warning call exceeds 120 characters ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The added get_logger().warning(...) statement is longer than the configured 120-character maximum.
Additional new test assertions and mock setup lines also exceed that limit.
Code

pr_agent/git_providers/github_provider.py[793]

+                get_logger().warning(f"Dropped {dropped_count} invalid comments that could not be fixed. Paths: {dropped_paths}")
Relevance

●●● Strong

Recent same-file precedent accepted wrapping newly added code to satisfy Ruff’s 120-character limit.

PR-#2776

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rules 2694655 and 2694690 both set a 120-character maximum, matching pyproject.toml's Ruff
configuration. The added warning call at line 793 and several added test lines exceed that bound.

Rule 2694655: Enforce 120-character maximum line length in Python source per Ruff config
Rule 2694690: Enforce maximum line length of 120 characters
pr_agent/git_providers/github_provider.py[793-793]
tests/unittest/test_github_provider_fallback_verification.py[16-16]

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

## Issue description
New Python source lines exceed the 120-character maximum configured for Ruff.

## Issue Context
Wrap the logger calls, mock setup, context managers, and assertions using parentheses while preserving behavior.

## Fix Focus Areas
- pr_agent/git_providers/github_provider.py[793-796]
- tests/unittest/test_github_provider_fallback_verification.py[16-29]
- tests/unittest/test_github_provider_fallback_verification.py[42-51]

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


2. Added lines contain trailing whitespace ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
New blank and comprehension lines contain spaces immediately before their newline characters. This
introduces whitespace-check failures in both modified Python files.
Code

pr_agent/git_providers/github_provider.py[786]

+            
Relevance

●●● Strong

Recent same-file precedent accepted removing whitespace-only and trailing-space lines.

PR-#2424

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694658 prohibits spaces or tabs immediately before a line break. The added whitespace-only
line 786 and line 790's trailing space demonstrate the violation, with further whitespace-only
additions in the test file.

Rule 2694658: Disallow trailing whitespace in source files
pr_agent/git_providers/github_provider.py[786-790]
tests/unittest/test_github_provider_fallback_verification.py[17-17]

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

## Issue description
Several added lines contain trailing spaces.

## Issue Context
Strip spaces and tabs occurring after the final visible character, including whitespace-only blank lines in the production and test files.

## Fix Focus Areas
- pr_agent/git_providers/github_provider.py[786-790]
- tests/unittest/test_github_provider_fallback_verification.py[17-27]
- tests/unittest/test_github_provider_fallback_verification.py[43-49]

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


3. Test file has extra newline ✓ Resolved 📘 Rule violation ✧ Quality
Description
The newly added test ends with an additional blank line after its last statement instead of exactly
one trailing newline. This violates the source-file EOF requirement.
Code

tests/unittest/test_github_provider_fallback_verification.py[52]

+
Relevance

●●● Strong

This is a trivial deterministic EOF formatting fix, consistent with the repository’s accepted
whitespace hygiene enforcement.

PR-#2424

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694661 requires exactly one trailing newline and no extra newline-only line. The diff adds
blank line 52 after the final assertion on line 51.

Rule 2694661: Source files must end with a single trailing newline character
tests/unittest/test_github_provider_fallback_verification.py[51-52]

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 new test file has an extra newline-only line at EOF.

## Issue Context
End the file immediately after line 51 with one newline character and no additional blank line.

## Fix Focus Areas
- tests/unittest/test_github_provider_fallback_verification.py[51-52]

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


View medium (3)
4. test imports are misgrouped ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The new test places third-party and project imports before the standard-library unittest.mock
import, with no blank lines between sections. This violates the required isort section order.
Code

tests/unittest/test_github_provider_fallback_verification.py[R1-4]

+import pytest
+from pr_agent.git_providers.github_provider import GithubProvider
+from pr_agent.config_loader import get_settings
+from unittest.mock import MagicMock, patch
Relevance

●●● Strong

Recent test-file precedents accepted restoring standard-library-first and blank-line-separated
import grouping.

PR-#2637
PR-#2796

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694656 requires standard-library imports first, followed by third-party and local imports with
blank-line separation. The added block starts with pytest, then pr_agent, and places
unittest.mock last.

Rule 2694656: Group Python imports according to isort sections and order
tests/unittest/test_github_provider_fallback_verification.py[1-4]

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 new test import block does not follow the required standard-library, third-party, then local-project order.

## Issue Context
Place `unittest.mock` first, `pytest` in the third-party section, and `pr_agent` imports in the local section, with blank lines between sections.

## Fix Focus Areas
- tests/unittest/test_github_provider_fallback_verification.py[1-4]

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


5. patch paths use single quotes ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The new Python test uses single-quoted module path literals in both patch() calls. The checklist
requires double quotes for Python string literals when doing so does not require extra escaping.
Code

tests/unittest/test_github_provider_fallback_verification.py[R22-23]

+    with patch('pr_agent.git_providers.github_provider.get_logger') as mock_logger, \
+         patch('pr_agent.git_providers.github_provider.get_settings') as mock_settings:
Relevance

●●● Strong

Recent same-file precedent accepted converting newly added single-quoted strings to double quotes.

PR-#2796

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694657 requires double quotes for Python string literals except where escaping justifies
single quotes; these module paths contain no double quotes and need no such exception.

Rule 2694657: Use double quotes for all Python string literals
tests/unittest/test_github_provider_fallback_verification.py[22-23]
tests/unittest/test_github_provider_fallback_verification.py[44-45]

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 added `patch()` module paths use single-quoted Python string literals.

## Issue Context
Replace these delimiters with double quotes without changing the path values, and make the same correction in the second test.

## Fix Focus Areas
- tests/unittest/test_github_provider_fallback_verification.py[22-23]
- tests/unittest/test_github_provider_fallback_verification.py[44-45]

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


6. Failed conversions omit paths ✓ Resolved 🐞 Bug ◔ Observability
Description
dropped_paths infers conversion success from the original comment's fields instead of the fixer's
actual result, so a comment with start_line, start_side, or a suggestion is excluded even when
_try_fix_invalid_inline_comments catches an exception and drops it. The warning then reports a
positive dropped count with an incomplete or empty path list, defeating the purpose of this change
for failed conversions.
Code

pr_agent/git_providers/github_provider.py[R789-792]

+                dropped_paths = [
+                    c.get("path") for c, _ in invalid_comments 
+                    if not ("```suggestion" in c.get("body", "") or "start_line" in c or "start_side" in c)
+                ]
Relevance

●● Moderate

Specific correctness concern lacks a closely matching accepted or rejected precedent; semantic
behavior requires review.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The fixer catches any per-comment conversion exception and omits that comment from its return list,
while the new path filter excludes originals solely because they contain fields that normally
trigger modification. For example, an invalid comment with start_line but no body raises at
comment["body"], is counted as dropped, but its path is filtered out because start_line is
present.

pr_agent/git_providers/github_provider.py[787-793]
pr_agent/git_providers/github_provider.py[840-854]
tests/unittest/test_github_provider_fallback_verification.py[12-29]

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 dropped-path warning reconstructs failures from comment shape, but the fixer can catch an exception and return no fixed comment for a shape that the filter assumes was fixed. Track which original comments were successfully converted so every unsuccessful conversion contributes its path.

## Issue Context
`_try_fix_invalid_inline_comments` currently returns only fixed dictionaries, so the caller cannot reliably associate successes and failures with the original comments. Adjust its result or perform per-comment conversion with explicit success tracking; keep publication failures distinct from conversion failures.

## Fix Focus Areas
- pr_agent/git_providers/github_provider.py[778-793]
- pr_agent/git_providers/github_provider.py[832-855]
- tests/unittest/test_github_provider_fallback_verification.py[6-29]

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



Informational
7. Comments use narrative phrasing ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The new comments Should have warned ... describe expected behavior narratively rather than giving
an imperative instruction. Both test cases introduce this phrasing.
Code

tests/unittest/test_github_provider_fallback_verification.py[28]

+        # Should have warned about 1 dropped comment
Relevance

● Weak

Recent precedent rejected rewriting narrative behavioral comments into imperative phrasing.

PR-#2774
PR-#2785

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694688 requires behavior-describing comments to use imperative phrasing. `Should have warned
about 1 dropped comment` and its second-test counterpart are descriptive rather than imperative.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_github_provider_fallback_verification.py[28-28]
tests/unittest/test_github_provider_fallback_verification.py[50-50]

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

## Issue description
Two added comments use descriptive narrative phrasing rather than imperative phrasing.

## Issue Context
Rewrite the comments as concise instructions, such as `# Verify one dropped-comment warning.` and `# Verify two dropped-comment warnings.`

## Fix Focus Areas
- tests/unittest/test_github_provider_fallback_verification.py[28-28]
- tests/unittest/test_github_provider_fallback_verification.py[50-50]

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


Grey Divider

Qodo Logo

@qodo-code-review

Copy link
Copy Markdown
Contributor

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

@qodo-code-review

Copy link
Copy Markdown
Contributor

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

@@ -0,0 +1,67 @@
from unittest.mock import MagicMock, patch

import pytest

import pytest

from pr_agent.config_loader import get_settings

@IsmaelMartinez IsmaelMartinez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Both tests assert on the rendered message rather than just the code path, so a broken format string could not slip through, and both go red on revert against today's main.

You should know there is an overlap neither page mentions: #2875 opened two hours before yours against the same issue and the same function. Yours is the one going in because it is correct and tested; I have explained why on that one.

Thanks for the tests.

@IsmaelMartinez
IsmaelMartinez merged commit 05b40d5 into The-PR-Agent:main Aug 29, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: inline comments that verification rejects and the one-liner rewrite cannot fix are dropped with no log

4 participants