fix(suggestions): only offer applicable changes

[TLA020]

Summary

Only exposes an Apply change action when the proposed replacement matches the exact lines it would replace.

Scope

All providers.

The validation preserves blank-line positions and rejects reversed or out-of-range anchors, missing file content, unavailable existing code, source mismatches, and malformed non-string source values. Invalid suggestions are skipped without stopping later valid suggestions. When a provider omits full file content, changed lines are verified against the unified diff. Suggestions with valid anchors remain inline, while suggestions whose anchors cannot be verified are published as regular PR comments. Provider failures remain visible, and a loaded empty diff cache is reused without another provider request.

Validation

• 86 focused suggestion and provider tests pass.
• 2,048 unit tests pass, with 1 skipped and 1 expected failure.
• Configured import ordering and changed-line length checks pass.
• git diff --check passes.

Related to #2110.

[qodo-code-review]

PR Summary by Qodo

Only publish committable inline suggestions when they are safely applyable

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Prevent “Apply change” from being offered when it would splice code or delete lines.
• Publish advice-only suggestions as normal inline comments instead of dropping them.
• Add unit tests covering applyable vs non-applyable suggestion publishing paths.

Diagram

graph TD
  A["Model suggestions"] --> B["PRCodeSuggestions.push_inline_code_suggestions"] --> C{"Applyable?\n(range covers existing_code)"}
  B --> D["GitProvider diff_files (head_file)"]
  C -->|Yes| E["Publish '''suggestion block\n(Apply button)"] --> F["git_provider.publish_code_suggestions"]
  C -->|No| G["Publish plain comment\n(+ optional fenced code)"] --> F

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Exact range match (strict equality)

• ➕ More deterministic: require existing_code to exactly equal the anchored range (including line count).
• ➕ Avoids substring/in-order matching false positives.
• ➖ More brittle to whitespace/formatting differences and dedent behavior.
• ➖ May downgrade valid applyable suggestions unnecessarily.

2. Derive applyability from diff/hunk mapping

• ➕ More semantically correct for PRs with large refactors: validate against patch context rather than head_file string matching.
• ➕ Can better handle moved/rewritten code when line anchors drift.
• ➖ Higher implementation complexity; depends on robust diff parsing and context matching.
• ➖ Still needs fallback behavior when mapping fails.

Recommendation: Current approach is a good risk-reduction tradeoff: it preserves existing behavior when verification is impossible, while withholding “Apply change” precisely in the known corruption cases (range mismatch / out-of-bounds / no existing_code). Consider tightening the matching logic later if substring-based checks produce false positives in real repos.

Files changed (2) +165 / -8

Bug fix (1) +63 / -8

pr_code_suggestions.pyGate committable suggestion fences behind an applyability check +63/-8

Gate committable suggestion fences behind an applyability check

• Stops filtering out suggestions that have no replacement code and adjusts dual publishing to keep them. Inline publishing now emits a '''suggestion block only when improved_code exists and existing_code is verifiably covered by the anchored line range; otherwise it posts a plain inline comment (optionally including a regular fenced code block) to avoid unsafe “Apply change” behavior.

pr_agent/tools/pr_code_suggestions.py

Tests (1) +102 / -0

test_pr_code_suggestions_core.pyAdd unit tests for applyable vs non-applyable inline suggestion publishing +102/-0

Add unit tests for applyable vs non-applyable inline suggestion publishing

• Introduces helper fixtures for a mock provider with head_file content and asserts publishing behavior across scenarios: safe one-line replacements, multi-line existing_code mismatches, out-of-file anchors, advice-only suggestions, dual publishing retention, and the ‘cannot read file’ permissive fallback.

tests/unittest/test_pr_code_suggestions_core.py

[qodo-code-review]

Code Review by Qodo

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

1. Blank lines bypass range check ✓ Resolved 🐞 Bug ≡ Correctness

Description

The applyability check removes blank lines from both the anchored file slice and existing_code
before comparing them. Consequently, existing_code containing extra blank lines can match a
shorter physical anchor and be offered as a committable suggestion even though the existing code
does not occupy the anchored range one-for-one.

Code

pr_agent/tools/pr_code_suggestions.py[625]

+        Return whether a suggestion replaces its complete anchored range.

Evidence

The helper's documented contract is to determine whether a suggestion replaces its complete anchored
range, but both list comprehensions discard empty physical lines before equality. For example, an
anchor containing first() and second() on adjacent lines can compare equal to existing_code
containing first(), a blank line, and second(), allowing a committable suggestion despite the
mismatch in physical coverage.

pr_agent/tools/pr_code_suggestions.py[623-638]

Agent prompt

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

## Issue description
`is_applyable_suggestion` filters out blank lines before comparing `existing_code` with the anchored file range. This allows code spanning additional physical lines to compare equal to a shorter anchor, incorrectly retaining the Apply button.

## Issue Context
The purpose of this helper is to prove that the replacement covers the complete anchored range one-for-one. Whitespace normalization may remain appropriate, but removing blank lines destroys the physical line-count information required for that check.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[635-638]

Compare line-by-line while preserving blank-line positions, or explicitly validate the physical line counts before normalization. Add a regression test where `existing_code` contains an extra blank line relative to the anchored range and assert that the suggestion is published as a plain comment.

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

---

2. push_inline_code_suggestions comment not imperative ⊘ Outdated 📘 Rule violation ⚙ Maintainability

Description

New behavior-describing comments are written in narrative form (e.g., "is published" / "degrades")
instead of imperative phrasing. This violates the documentation/comment style requirement and
reduces consistency/readability.

Code

pr_agent/tools/pr_code_suggestions.py[R594-595]

+                    # An inapplicable suggestion degrades to a plain comment: the advice still reaches the right
+                    # line, but an Apply change button is not offered for a change that would corrupt the file.

Evidence

Rule 2694688 requires newly added/modified comments that describe behavior to be written in
imperative mood. The added comments at pr_code_suggestions.py:244-245 ("is published") and :594-595
("degrades") are narrative/descriptive, not imperative.

Rule 2694688: Docstrings and comments must use imperative phrasing
pr_agent/tools/pr_code_suggestions.py[244-245]
pr_agent/tools/pr_code_suggestions.py[594-595]

Agent prompt

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

## Issue description
Newly added comments that describe behavior should use imperative phrasing (commands), but the PR introduces narrative phrasing.

## Issue Context
Per compliance, newly added/modified docstrings and behavior-describing comments must be imperative (e.g., "Publish ...", "Degrade ...") rather than descriptive (e.g., "... is published", "... degrades").

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[244-245]
- pr_agent/tools/pr_code_suggestions.py[594-595]

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

---

3. Unloaded files lose Apply ✓ Resolved 🐞 Bug ≡ Correctness

Description

When a provider skips loading file content and represents it as an empty string,
_get_head_file_lines() returns an empty list, causing is_applyable_suggestion() to reject any
anchored range and downgrade otherwise-committable suggestions to plain comments. This is triggered
by GitHubProvider setting head content to "" in avoid_load / rate-limit paths.

Code

pr_agent/tools/pr_code_suggestions.py[R618-621]

+                else self.git_provider.get_diff_files()
+            for file in diff_files or []:
+                if file.filename and file.filename.strip() == relevant_file:
+                    return file.head_file.splitlines() if file.head_file is not None else None

Evidence

The new helper returns splitlines() for any non-None head_file, so an empty string becomes [];
is_applyable_suggestion() then rejects ranges where relevant_lines_end > len(file_lines).
GitHubProvider explicitly sets new_file_content_str = "" when it avoids loading content
(avoid_load) or is close to rate limit, which feeds into FilePatchInfo.head_file.

pr_agent/tools/pr_code_suggestions.py[614-621]
pr_agent/tools/pr_code_suggestions.py[635-641]
pr_agent/git_providers/github_provider.py[304-320]

Agent prompt

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

### Issue description
`_get_head_file_lines()` treats `head_file == ""` as a readable empty file (returns `[]`). Several git providers use `""` as a sentinel for *unloaded/unavailable* content (e.g., avoiding full fetch for large PRs or near rate limits), so `is_applyable_suggestion()` then rejects all suggestions as out-of-range and removes the committable/suggestion fence.

### Issue Context
This is not about truly-empty files (where rejecting out-of-range anchors is correct). It’s about distinguishing “empty because file is empty” vs “empty because provider skipped loading content”.

### Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[614-647]
- pr_agent/git_providers/github_provider.py[304-331]

### Notes / approach
- Prefer making providers represent *unavailable/unloaded* content as `None` (not `""`), while leaving real empty files as `""`.
- Then decide policy for `is_applyable_suggestion()` on `None` (e.g., fail-open to preserve prior committable behavior, or configurable), without affecting truly empty files.

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

---

View medium (2)

4. Empty file fails open ✓ Resolved 🐞 Bug ≡ Correctness

Description

_get_head_file_lines() returns None when head_file is an empty string, so is_applyable_suggestion()
treats a readable-but-empty file the same as an unreadable file and returns True (keeping the Apply
button). If any provider represents a successfully read empty file as "" (or leaves the placeholder
""), out-of-range or unsafe suggestions won’t be downgraded as intended.

Code

pr_agent/tools/pr_code_suggestions.py[R619-620]

+                if file.filename and file.filename.strip() == relevant_file:
+                    return file.head_file.splitlines() if file.head_file else None

Evidence

The new logic explicitly defaults to applyable when file lines can’t be read, but
_get_head_file_lines returns None for any falsy head_file including an empty string. Since
FilePatchInfo.head_file is a str and diff parsing initializes it to "", an empty file (or a
not-yet-populated placeholder) is indistinguishable from unreadable content, causing the
applicability guard to be skipped.

pr_agent/tools/pr_code_suggestions.py[613-640]
pr_agent/git_providers/diff_parsing.py[31-68]
pr_agent/algo/types.py[14-20]

Agent prompt

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

## Issue description
`_get_head_file_lines()` currently does `return file.head_file.splitlines() if file.head_file else None`. This conflates:
- **Unreadable/missing content** (should return `None` so `is_applyable_suggestion()` defaults to `True` and preserves legacy behavior), and
- **Empty but readable file** (should return `[]` so `is_applyable_suggestion()` can correctly reject any anchored range > 0).

## Issue Context
`FilePatchInfo.head_file` is a `str` and is initialized to `""` in diff parsing. Some providers may later populate it with real content, but a legitimately empty file is also `""`. The current truthiness check can therefore bypass applicability validation in exactly the scenario this PR tries to guard.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[613-641]
- pr_agent/git_providers/diff_parsing.py[31-68]
- tests/unittest/test_pr_code_suggestions_core.py[279-285]

## Proposed fix direction
- Change `_get_head_file_lines()` to treat `None` as unreadable and `""` as readable empty:
 - e.g., `if file.head_file is None: return None` else `return file.head_file.splitlines()`.
- Ensure the pipeline uses `None` (not `""`) for “cannot be read” wherever possible (or adjust the placeholder initialization in diff parsing).
- Update/replace `test_is_applyable_suggestion_defaults_to_true_when_the_file_cannot_be_read` to use `head_file=None` to reflect “unreadable” rather than “empty file.”

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

---

5. Single quotes in d.get() ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

New code introduces single-quoted Python string literals (e.g., d.get('improved_code'), '', and
['existing_code']) instead of double-quoted literals. This violates the project requirement for
double quotes and may cause style/lint inconsistencies across the codebase.

Code

pr_agent/tools/pr_code_suggestions.py[581]

+                new_code_snippet = (d.get('improved_code') or '').rstrip()

Evidence

PR Compliance ID 2694657 requires using double quotes for Python string literals (excluding
docstrings). The modified code uses single-quoted literals such as d.get('improved_code') and ''
on newly changed lines, violating the rule.

Rule 2694657: Use double quotes for all Python string literals
pr_agent/tools/pr_code_suggestions.py[581-581]
pr_agent/tools/pr_code_suggestions.py[246-247]

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 adds single-quoted Python string literals, but the compliance checklist requires double quotes for all (non-docstring) Python string literals.

## Issue Context
This affects newly added/modified lines such as `d.get('improved_code')` and index access like `['existing_code']`.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[241-250]
- pr_agent/tools/pr_code_suggestions.py[577-603]

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

---

6. Misleading fallback reason ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New

Description

push_inline_code_suggestions() always says the Apply button is withheld because the suggestion does
not replace the anchored range one-for-one, but is_applyable_suggestion() also returns False for
other reasons (missing existing_code, missing file content, or out-of-range anchors). This produces
incorrect user-facing diagnostics and makes it harder to understand why the suggestion is not
committable.

Code

pr_agent/tools/pr_code_suggestions.py[R593-596]

+                    body = header
+                    if new_code_snippet:
+                        body += ("\n\nProposed code (not offered as a committable change, because it does not "
+                                 f"replace lines {relevant_lines_start}-{relevant_lines_end} one-for-one):\n"

Evidence

The fallback message hard-codes a one-for-one replacement explanation, but is_applyable_suggestion()
returns False for several other conditions unrelated to one-for-one replacement.

pr_agent/tools/pr_code_suggestions.py[586-597]
pr_agent/tools/pr_code_suggestions.py[623-634]

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 fallback comment text claims the suggestion is “not offered as a committable change” specifically because it does not replace the anchored range one-for-one. However, `is_applyable_suggestion()` can return `False` for multiple other reasons (file content unavailable, missing `existing_code`, anchored range out of bounds). This makes the published message incorrect in many cases.

## Issue Context
`push_inline_code_suggestions()` decides whether to use a ```suggestion fence based on `is_applyable_suggestion(...)`. When that returns `False`, the code currently emits a fixed explanation that only corresponds to one failure mode.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[586-597]
- pr_agent/tools/pr_code_suggestions.py[623-634]

## Suggested fix approach
Option A (simple): Replace the current explanatory sentence with a neutral one, e.g. “Proposed code (not offered as a committable change):”.

Option B (better): Have `is_applyable_suggestion()` return `(bool, reason)` (or an enum/string), and tailor the message:
- mismatched anchored content -> “does not replace the anchored range”
- missing file content -> “file content unavailable to verify applyability”
- missing existing_code -> “no existing_code provided to verify applyability”
- out-of-range anchor -> “anchor is outside file bounds”

Ensure the user-facing message accurately reflects the actual failure reason.

ⓘ 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: This push changes runtime suggestion-validation and diff-provider semantics across several code paths, creating real correctness risk beyond a single localized edit, but not enough independent logic to warrant extended review.

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 ↗

Previous reviews

Review updated until commit bc761b5

Results up to commit 85b1420 ⚖️ Balanced

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

1. Single quotes in d.get() ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

New code introduces single-quoted Python string literals (e.g., d.get('improved_code'), '', and
['existing_code']) instead of double-quoted literals. This violates the project requirement for
double quotes and may cause style/lint inconsistencies across the codebase.

Code

pr_agent/tools/pr_code_suggestions.py[581]

+                new_code_snippet = (d.get('improved_code') or '').rstrip()

Evidence

PR Compliance ID 2694657 requires using double quotes for Python string literals (excluding
docstrings). The modified code uses single-quoted literals such as d.get('improved_code') and ''
on newly changed lines, violating the rule.

Rule 2694657: Use double quotes for all Python string literals
pr_agent/tools/pr_code_suggestions.py[581-581]
pr_agent/tools/pr_code_suggestions.py[246-247]

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 adds single-quoted Python string literals, but the compliance checklist requires double quotes for all (non-docstring) Python string literals.

## Issue Context
This affects newly added/modified lines such as `d.get('improved_code')` and index access like `['existing_code']`.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[241-250]
- pr_agent/tools/pr_code_suggestions.py[577-603]

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

---

2. Empty file fails open ✓ Resolved 🐞 Bug ≡ Correctness

Description

_get_head_file_lines() returns None when head_file is an empty string, so is_applyable_suggestion()
treats a readable-but-empty file the same as an unreadable file and returns True (keeping the Apply
button). If any provider represents a successfully read empty file as "" (or leaves the placeholder
""), out-of-range or unsafe suggestions won’t be downgraded as intended.

Code

pr_agent/tools/pr_code_suggestions.py[R619-620]

+                if file.filename and file.filename.strip() == relevant_file:
+                    return file.head_file.splitlines() if file.head_file else None

Evidence

The new logic explicitly defaults to applyable when file lines can’t be read, but
_get_head_file_lines returns None for any falsy head_file including an empty string. Since
FilePatchInfo.head_file is a str and diff parsing initializes it to "", an empty file (or a
not-yet-populated placeholder) is indistinguishable from unreadable content, causing the
applicability guard to be skipped.

pr_agent/tools/pr_code_suggestions.py[613-640]
pr_agent/git_providers/diff_parsing.py[31-68]
pr_agent/algo/types.py[14-20]

Agent prompt

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

## Issue description
`_get_head_file_lines()` currently does `return file.head_file.splitlines() if file.head_file else None`. This conflates:
- **Unreadable/missing content** (should return `None` so `is_applyable_suggestion()` defaults to `True` and preserves legacy behavior), and
- **Empty but readable file** (should return `[]` so `is_applyable_suggestion()` can correctly reject any anchored range > 0).

## Issue Context
`FilePatchInfo.head_file` is a `str` and is initialized to `""` in diff parsing. Some providers may later populate it with real content, but a legitimately empty file is also `""`. The current truthiness check can therefore bypass applicability validation in exactly the scenario this PR tries to guard.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[613-641]
- pr_agent/git_providers/diff_parsing.py[31-68]
- tests/unittest/test_pr_code_suggestions_core.py[279-285]

## Proposed fix direction
- Change `_get_head_file_lines()` to treat `None` as unreadable and `""` as readable empty:
 - e.g., `if file.head_file is None: return None` else `return file.head_file.splitlines()`.
- Ensure the pipeline uses `None` (not `""`) for “cannot be read” wherever possible (or adjust the placeholder initialization in diff parsing).
- Update/replace `test_is_applyable_suggestion_defaults_to_true_when_the_file_cannot_be_read` to use `head_file=None` to reflect “unreadable” rather than “empty file.”

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

---

Results up to commit 67eda35 ⚖️ Balanced

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

1. Unloaded files lose Apply ✓ Resolved 🐞 Bug ≡ Correctness

Description

When a provider skips loading file content and represents it as an empty string,
_get_head_file_lines() returns an empty list, causing is_applyable_suggestion() to reject any
anchored range and downgrade otherwise-committable suggestions to plain comments. This is triggered
by GitHubProvider setting head content to "" in avoid_load / rate-limit paths.

Code

pr_agent/tools/pr_code_suggestions.py[R618-621]

+                else self.git_provider.get_diff_files()
+            for file in diff_files or []:
+                if file.filename and file.filename.strip() == relevant_file:
+                    return file.head_file.splitlines() if file.head_file is not None else None

Evidence

The new helper returns splitlines() for any non-None head_file, so an empty string becomes [];
is_applyable_suggestion() then rejects ranges where relevant_lines_end > len(file_lines).
GitHubProvider explicitly sets new_file_content_str = "" when it avoids loading content
(avoid_load) or is close to rate limit, which feeds into FilePatchInfo.head_file.

pr_agent/tools/pr_code_suggestions.py[614-621]
pr_agent/tools/pr_code_suggestions.py[635-641]
pr_agent/git_providers/github_provider.py[304-320]

Agent prompt

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

### Issue description
`_get_head_file_lines()` treats `head_file == ""` as a readable empty file (returns `[]`). Several git providers use `""` as a sentinel for *unloaded/unavailable* content (e.g., avoiding full fetch for large PRs or near rate limits), so `is_applyable_suggestion()` then rejects all suggestions as out-of-range and removes the committable/suggestion fence.

### Issue Context
This is not about truly-empty files (where rejecting out-of-range anchors is correct). It’s about distinguishing “empty because file is empty” vs “empty because provider skipped loading content”.

### Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[614-647]
- pr_agent/git_providers/github_provider.py[304-331]

### Notes / approach
- Prefer making providers represent *unavailable/unloaded* content as `None` (not `""`), while leaving real empty files as `""`.
- Then decide policy for `is_applyable_suggestion()` on `None` (e.g., fail-open to preserve prior committable behavior, or configurable), without affecting truly empty files.

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

---

2. push_inline_code_suggestions comment not imperative ⊘ Outdated 📘 Rule violation ⚙ Maintainability

Description

New behavior-describing comments are written in narrative form (e.g., "is published" / "degrades")
instead of imperative phrasing. This violates the documentation/comment style requirement and
reduces consistency/readability.

Code

pr_agent/tools/pr_code_suggestions.py[R594-595]

+                    # An inapplicable suggestion degrades to a plain comment: the advice still reaches the right
+                    # line, but an Apply change button is not offered for a change that would corrupt the file.

Evidence

Rule 2694688 requires newly added/modified comments that describe behavior to be written in
imperative mood. The added comments at pr_code_suggestions.py:244-245 ("is published") and :594-595
("degrades") are narrative/descriptive, not imperative.

Rule 2694688: Docstrings and comments must use imperative phrasing
pr_agent/tools/pr_code_suggestions.py[244-245]
pr_agent/tools/pr_code_suggestions.py[594-595]

Agent prompt

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

## Issue description
Newly added comments that describe behavior should use imperative phrasing (commands), but the PR introduces narrative phrasing.

## Issue Context
Per compliance, newly added/modified docstrings and behavior-describing comments must be imperative (e.g., "Publish ...", "Degrade ...") rather than descriptive (e.g., "... is published", "... degrades").

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[244-245]
- pr_agent/tools/pr_code_suggestions.py[594-595]

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

---

Results up to commit 885dd19 🚀 Fast

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

1. Blank lines bypass range check ✓ Resolved 🐞 Bug ≡ Correctness

Description

The applyability check removes blank lines from both the anchored file slice and existing_code
before comparing them. Consequently, existing_code containing extra blank lines can match a
shorter physical anchor and be offered as a committable suggestion even though the existing code
does not occupy the anchored range one-for-one.

Code

pr_agent/tools/pr_code_suggestions.py[625]

+        Return whether a suggestion replaces its complete anchored range.

Evidence

The helper's documented contract is to determine whether a suggestion replaces its complete anchored
range, but both list comprehensions discard empty physical lines before equality. For example, an
anchor containing first() and second() on adjacent lines can compare equal to existing_code
containing first(), a blank line, and second(), allowing a committable suggestion despite the
mismatch in physical coverage.

pr_agent/tools/pr_code_suggestions.py[623-638]

Agent prompt

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

## Issue description
`is_applyable_suggestion` filters out blank lines before comparing `existing_code` with the anchored file range. This allows code spanning additional physical lines to compare equal to a shorter anchor, incorrectly retaining the Apply button.

## Issue Context
The purpose of this helper is to prove that the replacement covers the complete anchored range one-for-one. Whitespace normalization may remain appropriate, but removing blank lines destroys the physical line-count information required for that check.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[635-638]

Compare line-by-line while preserving blank-line positions, or explicitly validate the physical line counts before normalization. Add a regression test where `existing_code` contains an extra blank line relative to the anchored range and assert that the suggestion is published as a plain comment.

ⓘ 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 67eda35

[qodo-code-review]

Code review by qodo was updated up to the latest commit 885dd19

[qodo-code-review]

Code review by qodo was updated up to the latest commit 740fad4

[qodo-code-review]

Code Review by Qodo

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

1. Indentation mismatches pass validation ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

_validate_suggestion strips every source line independently, so structurally different snippets
such as if x:\n    run() and if x:\nrun() compare equal. This exposes a committable Apply action
even though existing_code does not match the anchored lines' relative indentation.

Code

pr_agent/tools/pr_code_suggestions.py[R764-765]

+        anchored_lines = [line.strip() for line in anchored_lines]
+        existing_lines = [line.strip() for line in existing_code.splitlines()]

Relevance

●●● Strong

Deterministic correctness bug: stripping each line independently loses relative indentation needed
for valid comparison.

PR-#2679

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed comparison applies strip() separately to every anchored and generated line, which
removes all leading and trailing whitespace. The suggestion prompt explicitly requires
existing_code to preserve indentation, newlines, and original formatting, so relative-indentation
mismatches are source mismatches rather than applicable replacements.

pr_agent/tools/pr_code_suggestions.py[743-768]
pr_agent/settings/code_suggestions/pr_code_suggestions_prompts_not_decoupled.toml[98-100]

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 applicability check strips each line independently, erasing relative indentation and allowing structurally different source snippets to match.

## Issue Context
A common outer indentation may be normalized because model snippets can omit the file-level indent, but indentation differences between lines must remain significant.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[764-765]
- tests/unittest/test_pr_code_suggestions_core.py[568-573]

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

---

2. Load Gitea's initial diff cache ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

_get_diff_file treats any non-None cache as loaded, but Gitea initializes diff_files to an
empty list before its first get_diff_files() call. Consequently every Gitea suggestion gets "file
content is unavailable" and is downgraded to a PR-level comment rather than becoming a committable
inline suggestion.

Code

pr_agent/tools/pr_code_suggestions.py[R710-712]

+        diff_files = getattr(self.git_provider, "diff_files", None)
+        if diff_files is None:
+            diff_files = self.git_provider.get_diff_files()

Relevance

●●● Strong

Recent precedent fixes cache-truthiness bugs distinguishing unloaded None from cached empty list.

PR-#2381

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed lookup only calls the provider when the cache is None. Gitea constructs the cache as
[], and its loader explicitly proceeds to build a local diff_files list whenever that cache is
empty; therefore the changed lookup bypasses Gitea's loader and cannot locate the requested file.

pr_agent/tools/pr_code_suggestions.py[709-716]
pr_agent/git_providers/gitea_provider.py[77-105]
pr_agent/git_providers/gitea_provider.py[522-565]

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 `None`-only cache test treats Gitea's constructor-created empty list as a loaded diff cache. Gitea does not populate that list until `get_diff_files()` is invoked, so validation cannot find any file and disables every Apply action.

## Issue Context
Gitea initializes `self.diff_files = []`, while its `get_diff_files()` method uses an empty list as the signal to build its `FilePatchInfo` entries. Preserve the no-refetch behavior for providers whose empty cache genuinely means an already-loaded empty diff.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[709-716]
- pr_agent/git_providers/gitea_provider.py[77-105]
- pr_agent/git_providers/gitea_provider.py[522-565]

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

---

3. Invalid anchors still drop comments ✓ Resolved 🐞 Bug ≡ Correctness

Description

The plain-code fallback changes only the body while retaining a reversed or out-of-file inline
range, so real providers can skip or fail the comment instead of preserving it as an ordinary
comment. This directly breaks the fallback for the invalid anchors that _suggestion_applyability
now detects.

Code

pr_agent/tools/pr_code_suggestions.py[R624-627]

+                    body = header
+                    if new_code_snippet:
+                        body += (f"\n\nProposed code (not offered as a committable change because {fallback_reason}):\n"
+                                 f"```\n{new_code_snippet}\n```")

Relevance

●●● Strong

Fallback publication bug directly undermines the PR’s stated invalid-anchor behavior; accepted
precedents preserve output across provider failures.

PR-#2404
PR-#2491

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The fallback body is appended while the original line range is copied unchanged into the publication
payload. Azure DevOps explicitly skips reversed ranges, and GitLab indexes head_file using the
unchanged start line and catches the resulting exception, while the new out-of-range test only
verifies a mocked payload.

pr_agent/tools/pr_code_suggestions.py[624-631]
pr_agent/git_providers/azuredevops_provider.py[99-108]
pr_agent/git_providers/gitlab_provider.py[749-765]
tests/unittest/test_pr_code_suggestions_core.py[253-264]

Agent prompt

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

## Issue description
Non-applicable suggestions with invalid or out-of-file anchors are rendered as plain code blocks but still sent through the inline suggestion API with the invalid range, causing providers to skip or drop them.

## Issue Context
Source mismatches with a valid anchor may remain inline, but invalid positional metadata must not be reused. Route those fallback comments through a provider-supported non-inline/file-level publication path, or derive a verified valid anchor, and add integration-style coverage that exercises provider validation rather than only a `MagicMock` payload.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[617-631]
- tests/unittest/test_pr_code_suggestions_core.py[253-264]

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

---

4. Partial head shifts anchors ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

_validate_suggestion treats every non-empty head_file as complete and indexes it using absolute
file line numbers. Mosaico reconstructs head_file only from hunk lines, so multi-hunk diffs with
omitted gaps reject valid later anchors or compare them against the wrong source lines instead of
using the patch's absolute hunk positions.

Code

pr_agent/tools/pr_code_suggestions.py[R751-755]

+        if diff_file.head_file:
+            file_lines = diff_file.head_file.splitlines()
+            if relevant_lines_end > len(file_lines):
+                return False, "the anchored range is outside the file", False
+            anchored_lines = file_lines[relevant_lines_start - 1:relevant_lines_end]

Relevance

●●● Strong

Team accepts fixes for multi-hunk/absolute-position diff parsing correctness issues.

PR-#2137
PR-#2677

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Validation slices a truthy head_file by absolute anchors and only consults the patch when that
value is false. Mosaico's parser explicitly performs a best-effort reconstruction by appending only
lines encountered inside hunks, then stores that concatenation as head_file; therefore unchanged
gaps are not represented in its indexes.

pr_agent/tools/pr_code_suggestions.py[748-760]
pr_agent/mosaico/diff_provider.py[73-103]

Agent prompt

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

## Issue description
Validation indexes a partial, hunk-only `head_file` as though it represented the complete target file.

## Issue Context
The Mosaico diff provider reconstructs non-empty file content by concatenating hunk lines; unchanged gaps between hunks are absent, while the patch retains correct absolute target line numbers.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[751-760]
- pr_agent/mosaico/diff_provider.py[73-103]
- tests/unittest/test_pr_code_suggestions_core.py[533-556]

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

---

5. Docs imply all suggestions committable ✓ Resolved 📘 Rule violation ⚙ Maintainability ⭐ New

Description

Docs state that enabling --pr_code_suggestions.commitable_code_suggestions=true presents all
suggestions as committable, but the updated logic can publish some suggestions as non-committable
comments when the anchored range cannot be validated. This creates inaccurate user-facing
documentation for the improve tool behavior.

Code

pr_agent/tools/pr_code_suggestions.py[R681-686]

+            header = f"**Suggestion:** {content} [{label}, importance: {score}]" if score \
+                else f"**Suggestion:** {content} [{label}]"
+            if new_code_snippet and is_applicable:
+                body = f"{header}\n```suggestion\n" + new_code_snippet + "\n```"
+            else:
+                body = header

Relevance

●●● Strong

Recent precedent accepts doc fixes when comments/docs no longer match user-visible behavior.

PR-#2528

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new code builds a GitHub ``suggestion` block only when new_code_snippet` is present and the
suggestion is validated as applicable; otherwise it publishes a non-committable comment. The docs
currently claim you can "present all the suggestions as committable code comments" when enabling
--pr_code_suggestions.commitable_code_suggestions=true, which is no longer reliably true with this
behavior.

Rule 2694680: Update docs when user-facing behavior changes
pr_agent/tools/pr_code_suggestions.py[681-689]
docs/docs/tools/improve.md[24-36]

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 `improve` tool documentation states that all suggestions can be shown as committable code comments when `--pr_code_suggestions.commitable_code_suggestions=true`, but the updated implementation may publish some suggestions as plain PR comments when the anchored range/existing code cannot be verified.

## Issue Context
This PR introduces stricter applicability validation for committable suggestions, which is user-visible (some suggestions will no longer show an Apply action / GitHub suggestion block).

## Fix Focus Areas
- docs/docs/tools/improve.md[24-40]
- pr_agent/tools/pr_code_suggestions.py[681-689]

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

---

6. Advice-only malformed source escapes validation ✓ Resolved 🐞 Bug ≡ Correctness

Description

When improved_code is empty, the conditional assignment bypasses type validation for
existing_code, but the malformed value remains in original_suggestion. Bitbucket and Bitbucket
Server unconditionally call .rstrip() on that value and then skip the suggestion on failure, so
the advice-only comment is never published.

Code

pr_agent/tools/pr_code_suggestions.py[R660-662]

+                existing_code = d.get("existing_code") if new_code_snippet else None
+                if existing_code is not None and not isinstance(existing_code, str):
+                    raise TypeError("existing_code must be a string")

Relevance

●●● Strong

Accepted malformed-input handling precedent supports validating all provider-dereferenced fields,
including advice-only suggestions.

PR-#2314

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed guard only reads and validates existing_code when replacement text is non-empty, while
the publishing payload retains the original dictionary. Both Bitbucket implementations dereference
that retained field as a string and explicitly continue past the suggestion after an exception; the
focused test only exercises the default non-empty replacement, leaving this path uncovered.

pr_agent/tools/pr_code_suggestions.py[659-670]
pr_agent/tools/pr_code_suggestions.py[685-695]
pr_agent/git_providers/bitbucket_provider.py[168-184]
pr_agent/git_providers/bitbucket_server_provider.py[140-156]
tests/unittest/test_pr_code_suggestions_core.py[314-327]
PR-#2314

Agent prompt

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

## Issue description
Advice-only suggestions bypass `existing_code` type validation when `improved_code` is empty. Their unchanged `original_suggestion` then reaches Bitbucket providers, which call `.rstrip()` on the malformed value and skip publication.

## Issue Context
The validation should either reject every non-string `existing_code` before publishing or sanitize/remove the malformed field for advice-only suggestions. Extend the parameterized test to cover empty `improved_code` and assert the malformed suggestion does not reach provider publishing while later valid suggestions still do.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[660-662]
- tests/unittest/test_pr_code_suggestions_core.py[314-329]

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

---

View medium (3)

7. Malformed source aborts publishing ✓ Resolved 🐞 Bug ☼ Reliability

Description

The per-suggestion parse guard does not validate existing_code, and _validate_suggestion calls
splitlines() on any truthy value outside that guard. A YAML scalar or mapping in this
model-generated field therefore raises AttributeError and prevents all remaining suggestions from
being published.

Code

pr_agent/tools/pr_code_suggestions.py[R615-617]

+            is_applicable, fallback_reason, has_valid_anchor = self._validate_suggestion(
+                relevant_file, relevant_lines_start, relevant_lines_end,
+                d.get("existing_code") if new_code_snippet else None)

Relevance

●●● Strong

Accepted history favors guarding malformed non-string inputs; this deterministic type-safety fix
prevents suggestion-wide publishing failures.

PR-#2212
PR-#2569

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

AI output is parsed directly with yaml.safe_load, and preparation requires only the presence of
existing_code; it does not enforce a string type. The new parse guard ends before validation,
while _validate_suggestion unconditionally invokes existing_code.splitlines() for any truthy
value, so malformed YAML values escape the intended per-suggestion recovery.

pr_agent/algo/utils.py[753-768]
pr_agent/tools/pr_code_suggestions.py[576-582]
pr_agent/tools/pr_code_suggestions.py[601-617]
pr_agent/tools/pr_code_suggestions.py[703-708]

Agent prompt

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

## Issue description
A malformed non-string `existing_code` value reaches `_validate_suggestion`, where `splitlines()` raises outside the per-item parsing guard. This aborts the whole publishing operation instead of skipping only the malformed suggestion.

## Issue Context
Suggestion data comes from `yaml.safe_load`, which can produce integers, lists, or mappings, and `_prepare_pr_code_suggestions` checks only that the key exists. Validate the field's type in the existing per-suggestion parsing block without swallowing provider failures from diff loading.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[601-617]
- pr_agent/tools/pr_code_suggestions.py[703-708]
- tests/unittest/test_pr_code_suggestions_core.py[298-310]

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

---

8. Redundant applyability checks ✓ Resolved 🐞 Bug ➹ Performance

Description

push_inline_code_suggestions() calls _suggestion_applyability() even when improved_code is empty, so
advice-only suggestions still trigger diff-file lookup and range validation despite never producing
a committable ``suggestion`` block. This adds avoidable overhead (and may trigger get_diff_files()
when diff_files isn’t already available) for the new “advice-only suggestions are published”
behavior.

Code

pr_agent/tools/pr_code_suggestions.py[R587-590]

+                header = f"**Suggestion:** {content} [{label}, importance: {score}]" if score \
+                    else f"**Suggestion:** {content} [{label}]"
+                is_applicable, fallback_reason = self._suggestion_applyability(
+                    relevant_file, relevant_lines_start, relevant_lines_end, d.get("existing_code"))

Relevance

●●● Strong

Accepted history favors avoiding unnecessary expensive work; this is a local, deterministic guard
for advice-only suggestions.

PR-#2351
PR-#2404

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code invokes _suggestion_applyability() before checking whether there is any replacement code,
and _suggestion_applyability() performs file-content lookup via _get_head_file_lines(). Tests added
in this PR explicitly publish advice-only suggestions (improved_code=""), which will therefore still
take the validation path.

pr_agent/tools/pr_code_suggestions.py[572-597]
pr_agent/tools/pr_code_suggestions.py[611-639]
tests/unittest/test_pr_code_suggestions_core.py[254-266]

Agent prompt

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

## Issue description
`push_inline_code_suggestions()` computes `is_applicable, fallback_reason = self._suggestion_applyability(...)` for every suggestion, even when `improved_code` is empty. For advice-only suggestions, this work is unnecessary because the result is never used to render a committable ` ```suggestion``` ` block.

## Issue Context
This PR intentionally keeps/publishes advice-only suggestions (no replacement code). Those now take the same validation path, which can scan `diff_files` and may call `git_provider.get_diff_files()` when `diff_files` is not already present.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[583-597]

### Implementation sketch
- Only call `_suggestion_applyability(...)` inside an `if new_code_snippet:` branch.
- When `new_code_snippet` is empty, skip applyability validation and set `body = header` directly.
- Preserve current behavior for non-empty `new_code_snippet` (committable when applicable; otherwise add the fallback reason text).

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

---

9. Empty diff_files refetched ✓ Resolved 🐞 Bug ➹ Performance

Description

_get_head_file_lines() treats an empty cached git_provider.diff_files as a cache miss and calls
get_diff_files() again, which can trigger repeated expensive diff loading/API calls for providers
that also use truthy cache guards. This can add significant overhead when processing many
suggestions (or repeatedly checking applyability) in PRs where diff_files is legitimately empty.

Code

pr_agent/tools/pr_code_suggestions.py[R614-615]

+            diff_files = self.git_provider.diff_files if self.git_provider.diff_files \
+                else self.git_provider.get_diff_files()

Relevance

●●● Strong

Exact accepted precedent fixes empty diff_files caching with an is-not-None guard.

PR-#2381

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper uses a truthy check that will refetch when diff_files is []. Some providers treat
an empty list as a valid cached value (AzureDevOpsProvider uses is not None), while others
(CodeCommitProvider) use a truthy guard and will recompute when diff_files is empty—making repeated
calls expensive.

pr_agent/tools/pr_code_suggestions.py[611-616]
pr_agent/git_providers/codecommit_provider.py[112-117]
pr_agent/git_providers/azuredevops_provider.py[405-417]
PR-#2381

Agent prompt

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

### Issue description
`_get_head_file_lines()` uses a truthy check for `git_provider.diff_files`, so an empty list (`[]`) is treated as “not cached” and forces another `get_diff_files()` call. On providers that also use truthy cache guards (e.g., CodeCommitProvider), this can repeatedly rebuild/refetch diffs.

### Issue Context
This is in the newly added helper used by `_suggestion_applyability()` and can run once per suggestion.

### Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[611-616]

### Proposed fix
- Change the cache guard to distinguish “not loaded” from “loaded but empty”, e.g.:
 - `diff_files = getattr(self.git_provider, "diff_files", None)`
 - `if diff_files is None: diff_files = self.git_provider.get_diff_files()`
- Keep behavior the same when `diff_files` is an empty list (do not refetch).

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

---

10. Broad exception swallowing hides provider errors ✓ Resolved 🐞 Bug ◔ Observability

Description

_get_diff_file wraps the whole diff-files lookup (including attribute access and get_diff_files()
call) in a bare except Exception, converting any provider bug (e.g. AttributeError, malformed
FilePatchInfo) into a generic 'file content unavailable' warning. This silently downgrades
otherwise-valid suggestions to non-committable comments and makes real defects hard to detect since
only a warning log is produced with no traceback.

Code

pr_agent/tools/pr_code_suggestions.py[R641-651]

+    def _get_diff_file(self, relevant_file):
+        try:
+            diff_files = getattr(self.git_provider, "diff_files", None)
+            if diff_files is None:
+                diff_files = self.git_provider.get_diff_files()
+            for file in diff_files or []:
+                if file.filename and file.filename.strip() == relevant_file:
+                    return file
+        except Exception as e:
+            get_logger().warning(f"Could not read the file content of {relevant_file}, error: {e}")
+        return None

Relevance

●● Moderate

Team has accepted narrowing broad exception handling, but closely matching fail-open observability
findings were also rejected.

PR-#2231
PR-#2411
PR-#2424

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The try/except at lines 642-650 catches all exceptions from getattr, get_diff_files(), and the
iteration/attribute access on each file, reducing any unexpected failure to a warning-level log and
a None return that _suggestion_applyability interprets as 'the file content is unavailable',
silently degrading the suggestion instead of surfacing the root cause.

pr_agent/tools/pr_code_suggestions.py[641-651]

---

• 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: This push changes runtime suggestion-validation and diff-provider semantics across several code paths, creating real correctness risk beyond a single localized edit, but not enough independent logic to warrant extended review.

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 ↗

Previous reviews

Review updated until commit bc761b5

Results up to commit a2ea25e ⚖️ Balanced

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

1. Empty diff_files refetched ✓ Resolved 🐞 Bug ➹ Performance

Description

_get_head_file_lines() treats an empty cached git_provider.diff_files as a cache miss and calls
get_diff_files() again, which can trigger repeated expensive diff loading/API calls for providers
that also use truthy cache guards. This can add significant overhead when processing many
suggestions (or repeatedly checking applyability) in PRs where diff_files is legitimately empty.

Code

pr_agent/tools/pr_code_suggestions.py[R614-615]

+            diff_files = self.git_provider.diff_files if self.git_provider.diff_files \
+                else self.git_provider.get_diff_files()

Relevance

●●● Strong

Exact accepted precedent fixes empty diff_files caching with an is-not-None guard.

PR-#2381

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper uses a truthy check that will refetch when diff_files is []. Some providers treat
an empty list as a valid cached value (AzureDevOpsProvider uses is not None), while others
(CodeCommitProvider) use a truthy guard and will recompute when diff_files is empty—making repeated
calls expensive.

pr_agent/tools/pr_code_suggestions.py[611-616]
pr_agent/git_providers/codecommit_provider.py[112-117]
pr_agent/git_providers/azuredevops_provider.py[405-417]
PR-#2381

Agent prompt

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

### Issue description
`_get_head_file_lines()` uses a truthy check for `git_provider.diff_files`, so an empty list (`[]`) is treated as “not cached” and forces another `get_diff_files()` call. On providers that also use truthy cache guards (e.g., CodeCommitProvider), this can repeatedly rebuild/refetch diffs.

### Issue Context
This is in the newly added helper used by `_suggestion_applyability()` and can run once per suggestion.

### Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[611-616]

### Proposed fix
- Change the cache guard to distinguish “not loaded” from “loaded but empty”, e.g.:
 - `diff_files = getattr(self.git_provider, "diff_files", None)`
 - `if diff_files is None: diff_files = self.git_provider.get_diff_files()`
- Keep behavior the same when `diff_files` is an empty list (do not refetch).

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

---

Results up to commit c6bc35d ⚖️ Balanced

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

1. Redundant applyability checks ✓ Resolved 🐞 Bug ➹ Performance

Description

push_inline_code_suggestions() calls _suggestion_applyability() even when improved_code is empty, so
advice-only suggestions still trigger diff-file lookup and range validation despite never producing
a committable ``suggestion`` block. This adds avoidable overhead (and may trigger get_diff_files()
when diff_files isn’t already available) for the new “advice-only suggestions are published”
behavior.

Code

pr_agent/tools/pr_code_suggestions.py[R587-590]

+                header = f"**Suggestion:** {content} [{label}, importance: {score}]" if score \
+                    else f"**Suggestion:** {content} [{label}]"
+                is_applicable, fallback_reason = self._suggestion_applyability(
+                    relevant_file, relevant_lines_start, relevant_lines_end, d.get("existing_code"))

Relevance

●●● Strong

Accepted history favors avoiding unnecessary expensive work; this is a local, deterministic guard
for advice-only suggestions.

PR-#2351
PR-#2404

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code invokes _suggestion_applyability() before checking whether there is any replacement code,
and _suggestion_applyability() performs file-content lookup via _get_head_file_lines(). Tests added
in this PR explicitly publish advice-only suggestions (improved_code=""), which will therefore still
take the validation path.

pr_agent/tools/pr_code_suggestions.py[572-597]
pr_agent/tools/pr_code_suggestions.py[611-639]
tests/unittest/test_pr_code_suggestions_core.py[254-266]

Agent prompt

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

## Issue description
`push_inline_code_suggestions()` computes `is_applicable, fallback_reason = self._suggestion_applyability(...)` for every suggestion, even when `improved_code` is empty. For advice-only suggestions, this work is unnecessary because the result is never used to render a committable ` ```suggestion``` ` block.

## Issue Context
This PR intentionally keeps/publishes advice-only suggestions (no replacement code). Those now take the same validation path, which can scan `diff_files` and may call `git_provider.get_diff_files()` when `diff_files` is not already present.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[583-597]

### Implementation sketch
- Only call `_suggestion_applyability(...)` inside an `if new_code_snippet:` branch.
- When `new_code_snippet` is empty, skip applyability validation and set `body = header` directly.
- Preserve current behavior for non-empty `new_code_snippet` (committable when applicable; otherwise add the fallback reason text).

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

---

Results up to commit 25b6402 🧠 Deep

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

1. Invalid anchors still drop comments ✓ Resolved 🐞 Bug ≡ Correctness

Description

The plain-code fallback changes only the body while retaining a reversed or out-of-file inline
range, so real providers can skip or fail the comment instead of preserving it as an ordinary
comment. This directly breaks the fallback for the invalid anchors that _suggestion_applyability
now detects.

Code

pr_agent/tools/pr_code_suggestions.py[R624-627]

+                    body = header
+                    if new_code_snippet:
+                        body += (f"\n\nProposed code (not offered as a committable change because {fallback_reason}):\n"
+                                 f"```\n{new_code_snippet}\n```")

Relevance

●●● Strong

Fallback publication bug directly undermines the PR’s stated invalid-anchor behavior; accepted
precedents preserve output across provider failures.

PR-#2404
PR-#2491

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The fallback body is appended while the original line range is copied unchanged into the publication
payload. Azure DevOps explicitly skips reversed ranges, and GitLab indexes head_file using the
unchanged start line and catches the resulting exception, while the new out-of-range test only
verifies a mocked payload.

pr_agent/tools/pr_code_suggestions.py[624-631]
pr_agent/git_providers/azuredevops_provider.py[99-108]
pr_agent/git_providers/gitlab_provider.py[749-765]
tests/unittest/test_pr_code_suggestions_core.py[253-264]

Agent prompt

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

## Issue description
Non-applicable suggestions with invalid or out-of-file anchors are rendered as plain code blocks but still sent through the inline suggestion API with the invalid range, causing providers to skip or drop them.

## Issue Context
Source mismatches with a valid anchor may remain inline, but invalid positional metadata must not be reused. Route those fallback comments through a provider-supported non-inline/file-level publication path, or derive a verified valid anchor, and add integration-style coverage that exercises provider validation rather than only a `MagicMock` payload.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[617-631]
- tests/unittest/test_pr_code_suggestions_core.py[253-264]

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

---

2. Broad exception swallowing hides provider errors ✓ Resolved 🐞 Bug ◔ Observability

Description

_get_diff_file wraps the whole diff-files lookup (including attribute access and get_diff_files()
call) in a bare except Exception, converting any provider bug (e.g. AttributeError, malformed
FilePatchInfo) into a generic 'file content unavailable' warning. This silently downgrades
otherwise-valid suggestions to non-committable comments and makes real defects hard to detect since
only a warning log is produced with no traceback.

Code

pr_agent/tools/pr_code_suggestions.py[R641-651]

+    def _get_diff_file(self, relevant_file):
+        try:
+            diff_files = getattr(self.git_provider, "diff_files", None)
+            if diff_files is None:
+                diff_files = self.git_provider.get_diff_files()
+            for file in diff_files or []:
+                if file.filename and file.filename.strip() == relevant_file:
+                    return file
+        except Exception as e:
+            get_logger().warning(f"Could not read the file content of {relevant_file}, error: {e}")
+        return None

Relevance

●● Moderate

Team has accepted narrowing broad exception handling, but closely matching fail-open observability
findings were also rejected.

PR-#2231
PR-#2411
PR-#2424

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The try/except at lines 642-650 catches all exceptions from getattr, get_diff_files(), and the
iteration/attribute access on each file, reducing any unexpected failure to a warning-level log and
a None return that _suggestion_applyability interprets as 'the file content is unavailable',
silently degrading the suggestion instead of surfacing the root cause.

pr_agent/tools/pr_code_suggestions.py[641-651]

---

Results up to commit 0d4d7c6 ⚖️ Balanced

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

1. Malformed source aborts publishing ✓ Resolved 🐞 Bug ☼ Reliability

Description

The per-suggestion parse guard does not validate existing_code, and _validate_suggestion calls
splitlines() on any truthy value outside that guard. A YAML scalar or mapping in this
model-generated field therefore raises AttributeError and prevents all remaining suggestions from
being published.

Code

pr_agent/tools/pr_code_suggestions.py[R615-617]

+            is_applicable, fallback_reason, has_valid_anchor = self._validate_suggestion(
+                relevant_file, relevant_lines_start, relevant_lines_end,
+                d.get("existing_code") if new_code_snippet else None)

Relevance

●●● Strong

Accepted history favors guarding malformed non-string inputs; this deterministic type-safety fix
prevents suggestion-wide publishing failures.

PR-#2212
PR-#2569

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

AI output is parsed directly with yaml.safe_load, and preparation requires only the presence of
existing_code; it does not enforce a string type. The new parse guard ends before validation,
while _validate_suggestion unconditionally invokes existing_code.splitlines() for any truthy
value, so malformed YAML values escape the intended per-suggestion recovery.

pr_agent/algo/utils.py[753-768]
pr_agent/tools/pr_code_suggestions.py[576-582]
pr_agent/tools/pr_code_suggestions.py[601-617]
pr_agent/tools/pr_code_suggestions.py[703-708]

Agent prompt

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

## Issue description
A malformed non-string `existing_code` value reaches `_validate_suggestion`, where `splitlines()` raises outside the per-item parsing guard. This aborts the whole publishing operation instead of skipping only the malformed suggestion.

## Issue Context
Suggestion data comes from `yaml.safe_load`, which can produce integers, lists, or mappings, and `_prepare_pr_code_suggestions` checks only that the key exists. Validate the field's type in the existing per-suggestion parsing block without swallowing provider failures from diff loading.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[601-617]
- pr_agent/tools/pr_code_suggestions.py[703-708]
- tests/unittest/test_pr_code_suggestions_core.py[298-310]

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

---

Results up to commit 37678bb 🧠 Deep

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

1. Advice-only malformed source escapes validation ✓ Resolved 🐞 Bug ≡ Correctness

Description

When improved_code is empty, the conditional assignment bypasses type validation for
existing_code, but the malformed value remains in original_suggestion. Bitbucket and Bitbucket
Server unconditionally call .rstrip() on that value and then skip the suggestion on failure, so
the advice-only comment is never published.

Code

pr_agent/tools/pr_code_suggestions.py[R660-662]

+                existing_code = d.get("existing_code") if new_code_snippet else None
+                if existing_code is not None and not isinstance(existing_code, str):
+                    raise TypeError("existing_code must be a string")

Relevance

●●● Strong

Accepted malformed-input handling precedent supports validating all provider-dereferenced fields,
including advice-only suggestions.

PR-#2314

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed guard only reads and validates existing_code when replacement text is non-empty, while
the publishing payload retains the original dictionary. Both Bitbucket implementations dereference
that retained field as a string and explicitly continue past the suggestion after an exception; the
focused test only exercises the default non-empty replacement, leaving this path uncovered.

pr_agent/tools/pr_code_suggestions.py[659-670]
pr_agent/tools/pr_code_suggestions.py[685-695]
pr_agent/git_providers/bitbucket_provider.py[168-184]
pr_agent/git_providers/bitbucket_server_provider.py[140-156]
tests/unittest/test_pr_code_suggestions_core.py[314-327]
PR-#2314

Agent prompt

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

## Issue description
Advice-only suggestions bypass `existing_code` type validation when `improved_code` is empty. Their unchanged `original_suggestion` then reaches Bitbucket providers, which call `.rstrip()` on the malformed value and skip publication.

## Issue Context
The validation should either reject every non-string `existing_code` before publishing or sanitize/remove the malformed field for advice-only suggestions. Extend the parameterized test to cover empty `improved_code` and assert the malformed suggestion does not reach provider publishing while later valid suggestions still do.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[660-662]
- tests/unittest/test_pr_code_suggestions_core.py[314-329]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 25b6402

[qodo-code-review]

Code review by qodo was updated up to the latest commit 0d4d7c6

[qodo-code-review]

Code review by qodo was updated up to the latest commit 37678bb

[qodo-code-review]

Code review by qodo was updated up to the latest commit 5aed97e

[qodo-code-review]

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

[qodo-code-review]

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

[IsmaelMartinez]

Verified end to end: merges cleanly onto main, suite green, and reverting the source while keeping your tests turns 25 of them red, so the coverage is real. The approach is right, and gating committability on the anchored range verifying against the head file is the durable fix. It complements #2626, which stops the model producing truncated improved_code in the first place.

Two questions inline before I approve. Thanks for contributing!

[IsmaelMartinez]

This no longer requires improved_code to admit a suggestion above the threshold, where main gates on and suggestion.get('improved_code'). Deliberate? Suggestions with no improved_code now reach the dual-publishing path where they were previously skipped.

[TLA020]

will come back on this one

[IsmaelMartinez]

This posts a fresh comment on every run. pr_agent/algo/inline_comment_dedup.py exists for exactly that pile-up, behind config.persistent_inline_comments. Worth routing through it, or is a per-run note the intent?

[TLA020]

hmmm will check & test

[IsmaelMartinez]

Sorry, my dedup suggestion above doesn't hold, please ignore it. inline_comment_dedup only reads review comments, and this path posts a plain PR comment, so the marker would never be read back.

The pile-up itself is still real. I got a bit confused with another PR, apologies.

[IsmaelMartinez]

Merging. The applicable-only filter is the right call, and the tests cover the anchor validation well.

One follow-up rather than a blocker: the new fallback_comments path posts a fresh PR comment on every run, so repeated /improve runs will stack them. publish_persistent_comment would update one comment in place instead. Happy to do that separately, or leave it to you if you'd rather.

[IsmaelMartinez]

Verified against current main: merges clean, 2444 passing locally.