[utsab345]

Fixes #3589

Problem

convert_to_markdown_v2 (pr_agent/algo/utils.py:318-319) called int(str(issue.get("start_line", 0)).strip()) inside a per-finding try/except Exception. When the model emitted a non-integer line field (empty string, N/A, a float, null, or a hyphenated range), the ValueError was caught and the entire finding was dropped from the published /review summary with no user-visible error.

The two sibling consumers of the same dict are tolerant and keep the finding:

• pr_agent/tools/pr_reviewer.py:1263-1266 (_build_key_issue_comment), inline comments.
• pr_agent/tools/pr_reviewer.py:648-651 (_review_finding_from_issue), persisted finding state.

So a "Possible security issue" could vanish from the summary while inline comments and the persistent state still recorded it, and the bookkeeping for later reviews diverged from what maintainers actually saw.

Fix

In convert_to_markdown_v2, parse start_line/end_line tolerantly (defaulting to 0 on non-numeric input, matching the sibling paths). When the lines are not usable (start_line < 1 or end_line < start_line), the finding still renders in the summary with its header and content, but without a line anchor link or excerpt. Calling extract_relevant_lines_str and get_line_link is gated on valid_lines so no bogus R0 anchor is produced.

Tests

Added test_key_issue_with_non_numeric_lines_keeps_finding_in_summary in tests/unittest/test_convert_to_markdown.py: a finding with start_line: "" still renders header and content and get_line_link is not called.

Verification:

• New test plus the reviewer, finding-state, and markdown render suites pass.
• Full unit suite: 9187 passed, 33 skipped, 1 xfailed.
• ruff check and the pre-commit hooks are clean on the touched files.

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

PR Summary by Qodo

Preserve review findings with invalid line metadata

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Preserve review findings when model-generated line fields are malformed.
• Omit excerpts and line links when findings lack a valid range.
• Add regression coverage for non-numeric line metadata.

Diagram

graph TD
    A["Review finding"] --> B["Parse line fields"] --> C{"Valid range?"}
    C -->|Yes| D["Extract excerpt"] --> E["Build line link"] --> F["Render summary"]
    C -->|No| F

Loading

High-Level Assessment

The localized tolerant parsing and explicit range validation are appropriate because they align summary behavior with existing inline-comment and persisted-state consumers. Extracting a shared parser was considered but would add cross-cutting refactoring for a small, isolated consistency fix.

Files changed (2) +31 / -5

Bug fix (1) +8 / -5

utils.pyRender findings despite malformed line fields +8/-5

Render findings despite malformed line fields

• Catches invalid start or end line values and falls back to an unlocated finding. Source extraction and provider line-link generation now occur only for positive, ordered ranges, preventing invalid anchors while preserving finding content.

pr_agent/algo/utils.py

Tests (1) +23 / -0

test_convert_to_markdown.pyCover findings with non-numeric line metadata +23/-0

Cover findings with non-numeric line metadata

• Adds a regression test proving that malformed line metadata does not remove the finding from the review summary. It also verifies that no provider line-link request is made without a valid range.

tests/unittest/test_convert_to_markdown.py

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

Code Review by Qodo

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

1. Malformed end lines remain untested ✓ Resolved 📎 Requirement gap ⚙ Maintainability

Description

test_key_issue_with_non_numeric_lines_keeps_finding_in_summary supplies only an empty
start_line; it never exercises a malformed end_line. Although an earlier test preserves the
valid integer path, None, floats, ranges, and other invalid values can reach either field without
the ticket-required coverage.

Code

tests/unittest/test_convert_to_markdown.py[R160-161]

+            'start_line': '',
+            'end_line': 14,

Evidence

The ticket-derived rule explicitly requires automated tests for valid and invalid values in both
line fields. The added test uses an invalid value only for start_line, while end_line remains
the valid integer 14.

Preserve review findings with non-numeric line values in the published summary
tests/unittest/test_convert_to_markdown.py[149-170]

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 regression test covers only an empty `start_line`, while the compliance requirement calls for invalid-value coverage of both `start_line` and `end_line`, including representative non-coercible forms.
## Fix Focus Areas
- tests/unittest/test_convert_to_markdown.py[149-170]
## Recommended Fix
Parameterize the test over both line fields and representative invalid values such as an empty string, `N/A`, `12.5`, `None`, and `12-15`. Assert that every case retains the finding content and does not request a line link, while preserving the existing valid-integer test.

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

---

2. Regression test mixes quote styles 📘 Rule violation ⚙ Maintainability

Description

test_key_issue_with_non_numeric_lines_keeps_finding_in_summary uses double-quoted parameter names
and values while its input and assertion literals use single quotes. Editing or extending this
parameter matrix now requires readers to reconcile two unexplained styles within one test block.

Code

tests/unittest/test_convert_to_markdown.py[R151-154]

+    @pytest.mark.parametrize("field,invalid_value", [
+        ("start_line", ""),
+        ("start_line", "N/A"),
+        ("start_line", 12.5),

Evidence

Compliance rule 2694657 requires each Python file to use one predominant quote style for comparable
simple literals. The newly added parameter matrix uses double quotes, while the newly added test
body uses single quotes for the corresponding field keys and other simple literals.

Rule 2694657: Use a single quote style per file for Python string literals
tests/unittest/test_convert_to_markdown.py[151-175]

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 parameter matrix uses double-quoted string literals while the remainder of the same test uses single-quoted literals.
## Fix Focus Areas
- tests/unittest/test_convert_to_markdown.py[151-161]
## Recommended Fix
Change the decorator argument and string values in the parameter matrix to single-quoted literals, preserving all parameter values and test behavior.

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

---

3. Two new lines exceed 120 characters 📘 Rule violation ⚙ Maintainability

Description

The ternary assignment in convert_to_markdown_v2 and the mock URL assignment in its regression
test each exceed the configured 120-character limit. Measuring the touched Python lines against
pyproject.toml therefore leaves both modified files outside the required source-line constraint.

Code

pr_agent/algo/utils.py[318]

+                        relevant_lines_str = extract_relevant_lines_str(end_line, files, relevant_file, start_line, dedent=True) if valid_lines else ""

Evidence

The repository config sets the line length to 120, while added lines 318 and 164 are longer than
that limit.

Rule 2694655: Limit Python source lines to 120 characters as configured
pyproject.toml[151-151]
pr_agent/algo/utils.py[318-318]
tests/unittest/test_convert_to_markdown.py[164-164]

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 Python lines exceed the repository's configured 120-character source-line limit.
## Fix Focus Areas
- pr_agent/algo/utils.py[318-318]
- tests/unittest/test_convert_to_markdown.py[164-164]
## Recommended Fix
Wrap the conditional assignment in parentheses across multiple lines and split the mock URL assignment using a parenthesized string or intermediate variable, keeping every physical line at or below 120 characters.

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

---

4. New fallback breaks local quote style 📘 Rule violation ⚙ Maintainability

Description

The new fallback in convert_to_markdown_v2 uses the double-quoted literal "" while surrounding
simple string literals consistently use single quotes. Later edits to this rendering branch must now
account for an unexplained one-line exception to the file's established style.

Code

pr_agent/algo/utils.py[318]

+                        relevant_lines_str = extract_relevant_lines_str(end_line, files, relevant_file, start_line, dedent=True) if valid_lines else ""

Evidence

The quote-style rule requires simple literals to follow the predominant style in each Python file.
The surrounding rendering code uses single quotes, but the new fallback uses double quotes without
an escaping need.

Rule 2694657: Use a single quote style per file for Python string literals
pr_agent/algo/utils.py[303-318]

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 empty-string fallback uses double quotes despite the surrounding file's predominant single-quote style.
## Fix Focus Areas
- pr_agent/algo/utils.py[318-318]
## Recommended Fix
Replace `""` with `''` while wrapping the assignment as needed to preserve the configured line length.

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

---

View medium (1)

5. New test docstring is not imperative 📘 Rule violation ⚙ Maintainability

Description

The added test docstring begins with the declarative requirement `A non-integer start_line/end_line
must not silently drop the finding` rather than an imperative summary. A later reader encounters a
policy statement instead of the action-oriented behavior description required for new docstrings.

Code

tests/unittest/test_convert_to_markdown.py[150]

+        """A non-integer start_line/end_line must not silently drop the finding.

Evidence

The rule requires newly added docstrings to begin with imperative phrasing. The added first line has
an explicit subject and modal predicate, so it is declarative rather than imperative.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_convert_to_markdown.py[150-155]

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 docstring begins with declarative phrasing rather than an imperative description of the tested behavior.
## Fix Focus Areas
- tests/unittest/test_convert_to_markdown.py[150-155]
## Recommended Fix
Rewrite the opening line in imperative mood, for example: `Keep findings with non-integer line values in the summary.` Preserve the remaining context if it is still useful.

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

Tip of the day

💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

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

Code review by qodo was updated up to the latest commit 2e40851

[IsmaelMartinez]

Thanks for filing #3589 with a clean reproduction and then fixing it, and for parametrising over both line fields when Qodo asked. Approving, which closes #3589.

Reverting utils.py underneath your tests turns the new cases red and leaves the rest green, so the drop was real and is now covered.

The gate also stops a line link being requested for missing or inverted ranges, which reads right, and nothing in the existing summary tests depended on that.