Skip to content

fix: YAML parser error from diff-style lines in nested lists - #2061

Merged
IsmaelMartinez merged 5 commits into
The-PR-Agent:mainfrom
isExample:fix/nested-diff-markers
Aug 28, 2026
Merged

fix: YAML parser error from diff-style lines in nested lists#2061
IsmaelMartinez merged 5 commits into
The-PR-Agent:mainfrom
isExample:fix/nested-diff-markers

Conversation

@isExample

@isExample isExample commented Oct 10, 2025

Copy link
Copy Markdown
Contributor

User description

While using pr-agent, I repeatedly ran into YAML parsing errors when using the describe or improve tools on GitHub PRs.
Here’s one of the error examples I encountered:
image

I found that the issue was caused by AI-generated diffs inserting diff-style prefixes (-, +, -+, etc.) inside YAML lists. This confused the parser and caused every fallback in try_fix_yaml to return None.

So I added a regression test reproducing the failure and implemented the 5.5 fallback to handle it.
As a result, the previously failing cases now pass cleanly:
image


PR Type

Bug fix


Description

  • Fix YAML parser error from diff-style markers in nested lists

  • Add fallback to normalize diff markers before parsing

  • Include regression test for diff marker handling


Diagram Walkthrough

flowchart LR
  A["AI generates YAML with diff markers"] --> B["try_fix_yaml function"]
  B --> C["New fallback 5.5 normalizes diff markers"]
  C --> D["YAML parses successfully"]
  E["Regression test"] --> F["Validates fix works"]
Loading

File Walkthrough

Relevant files
Bug fix
utils.py
Add YAML diff marker normalization fallback                           

pr_agent/algo/utils.py

  • Add fallback 5.5 to normalize diff-style markers in YAML
  • Handle lines starting with -, +, -+ patterns
  • Clean diff markers while preserving YAML list structure
  • Add logging for successful parsing after normalization
+34/-0   
Tests
test_try_fix_yaml.py
Add regression test for diff marker handling                         

tests/unittest/test_try_fix_yaml.py

  • Add regression test for diff markers in nested lists
  • Test YAML with -, +, -+ markers in list items
  • Verify expected output structure after normalization
+33/-0   

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Oct 10, 2025

Copy link
Copy Markdown
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Consistent Naming Conventions

Objective: All new variables, functions, and classes must follow the project's established naming
standards

Status: Passed

No Dead or Commented-Out Code

Objective: Keep the codebase clean by ensuring all submitted code is active and necessary

Status: Passed

When relevant, utilize early return

Objective: In a code snippet containing multiple logic conditions (such as 'if-else'), prefer an
early return on edge cases than deep nesting

Status: Passed

Robust Error Handling

Objective: Ensure potential errors and edge cases are anticipated and handled gracefully throughout
the code

Status:
Broad except: The new fallback uses bare except blocks without specifying exception types or logging
error details, which may mask parsing issues.

Referred Code
try:
    data = yaml.safe_load('\n'.join(response_text_lines_copy))
    get_logger().info("Successfully parsed AI prediction after normalizing diff removal markers")
    return data
except:
    pass
Single Responsibility for Functions

Objective: Each function should have a single, well-defined responsibility

Status:
Multi-purpose function: The added fallback increases the scope of try_fix_yaml with another transformation pass,
suggesting the function is handling many concerns.

Referred Code
# 5.5 fallback - try to normalize diff-style removal markers ('-') within list items
response_text_lines_copy = response_text_lines.copy()
modified = False

for i, line in enumerate(response_text_lines_copy):
    if line.startswith('+'):
        response_text_lines_copy[i] = ' ' + line[1:]
        modified = True

for i, line in enumerate(response_text_lines_copy):
    if not line.startswith('-'):
        continue
    remainder = line[1:]
    if line.startswith('- '):
        second_char = remainder[1] if len(remainder) > 1 else ''
        if second_char and second_char not in (' ', '\t', '+', '-'):
            continue
    cleaned = remainder
    while cleaned and cleaned[0] in ('+', '-'):
        cleaned = cleaned[1:]
    if cleaned and cleaned[0] not in (' ', '\t'):


 ... (clipped 10 lines)
  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@isExample

Copy link
Copy Markdown
Contributor Author

/review

@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
✅ No TODO sections
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The normalization compares cleaned to the original line but assigns cleaned back without the original leading '-' prefix, which may break list structure; verify yaml.safe_load still interprets list items correctly for lines beginning with '-' that are transformed.

for i, line in enumerate(response_text_lines_copy):
    if not line.startswith('-'):
        continue
    remainder = line[1:]
    if line.startswith('- '):
        second_char = remainder[1] if len(remainder) > 1 else ''
        if second_char and second_char not in (' ', '\t', '+', '-'):
            continue
    cleaned = remainder
    while cleaned and cleaned[0] in ('+', '-'):
        cleaned = cleaned[1:]
    if cleaned and cleaned[0] not in (' ', '\t'):
        cleaned = ' ' + cleaned
    if cleaned != line:
        response_text_lines_copy[i] = cleaned
        modified = True
Logging Clarity

The log message mentions normalizing removal markers but the code also strips leading '+' earlier; consider clarifying the message or splitting logs for '+' and '-' normalization to aid debugging.

if modified:
    try:
        data = yaml.safe_load('\n'.join(response_text_lines_copy))
        get_logger().info("Successfully parsed AI prediction after normalizing diff removal markers")
        return data
    except:

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Oct 10, 2025

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent corruption of valid YAML lists
Suggestion Impact:The commit adjusted the logic for lines starting with '-' to better distinguish real YAML list items from diff deletions, adding comments and preserving real list items (continue) before cleaning diff markers. This aligns with the suggestion’s intent to avoid corrupting valid list entries.

code diff:

+    # normalize lines starting with '-'. Distinguish real YAML list items from diff deletions.
     for i, line in enumerate(response_text_lines_copy):
         if not line.startswith('-'):
             continue
+
         remainder = line[1:]
         if line.startswith('- '):
             second_char = remainder[1] if len(remainder) > 1 else ''
             if second_char and second_char not in (' ', '\t', '+', '-'):
-                continue
+                continue # real list item → keep as-is
+
+        # treat it as a diff "removed" marker inside block content
         cleaned = remainder
         while cleaned and cleaned[0] in ('+', '-'):
             cleaned = cleaned[1:]

Modify the YAML cleaning logic to prevent the corruption of valid list items.
The current implementation incorrectly strips the leading - from lines like -
key: value, which breaks the YAML structure.

pr_agent/algo/utils.py [880-895]

 for i, line in enumerate(response_text_lines_copy):
     if not line.startswith('-'):
         continue
     remainder = line[1:]
     if line.startswith('- '):
         second_char = remainder[1] if len(remainder) > 1 else ''
         if second_char and second_char not in (' ', '\t', '+', '-'):
             continue
+        # If we are here, it's likely a valid list item, but it might have extra diff markers.
+        # Example: '- - key: value' or '- + key: value'
+        # However, if it's just '-  key: value', we should not process it further in this block.
+        if not (remainder.lstrip().startswith('+') or remainder.lstrip().startswith('-')):
+            continue
+
     cleaned = remainder
     while cleaned and cleaned[0] in ('+', '-'):
         cleaned = cleaned[1:]
     if cleaned and cleaned[0] not in (' ', '\t'):
         cleaned = ' ' + cleaned
     if cleaned != line:
         response_text_lines_copy[i] = cleaned
         modified = True

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug in the new code where valid YAML list items like - key: value would be corrupted. The proposed fix is logical and effectively resolves this critical issue, preventing potential YAML parsing failures.

Medium
Learned
best practice
Safely use optional logger

Ensure logger retrieval is resilient in environments where logging may be
stubbed by safely accessing get_logger() and method existence before use.

pr_agent/algo/utils.py [899]

-get_logger().info("Successfully parsed AI prediction after normalizing diff removal markers")
+logger = get_logger()
+if hasattr(logger, "info"):
+    logger.info("Successfully parsed AI prediction after normalizing diff removal markers")
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Guard access to nested settings and attributes with safe accessors and validation, providing defaults or clear errors when missing.

Low
  • Update
  • Author self-review: I have reviewed the PR code suggestions, and addressed the relevant ones.

@DanaFineTLV

Copy link
Copy Markdown
Collaborator

Hi @isExample
Apologies for the delayed response, and thank you for your questions and contributions 🙏

We offer a free Qodo version for free-trial for developers [www.qodo.ai],
and offer a free version of our paid product for open-source projects [https://www.qodo.ai/solutions/open-source/]. 🚀

We’re currently restructuring the project and contributing it to the community, with plans to move it under a foundation.
If you’re interested in taking part, please reach out to me at dana.f@qodo.ai
or via LinkedIn

We’ll also be launching an Ambassador Program soon — if you’d like to join, stay tuned for more details!

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

Thank you for this, and I am sorry it sat: 318 days with nobody looking at it. It is also still needed.

The bug reproduces on today's main. Your /improve shape does not fail loudly there, it silently loses data: a block scalar carrying - markers swallows the keys that follow it into itself. The same markers on the /review schema are worse. key_issues_to_review with - prefixed block content returns None outright on main, and parses correctly with your fallback applied.

Two small things if you are still around. Rebase: only test_try_fix_yaml.py conflicts, because main appended a test in the same place, so it is a keep-both. And widen the regression test to cover the /review shape, since that is the harder failure and the current test does not reach it.

If you would rather not pick it back up, say so and I will land it with credit to you.

IsmaelMartinez and others added 2 commits August 28, 2026 16:08
# Conflicts:
#	tests/unittest/test_try_fix_yaml.py
Matches the guard The-PR-Agent#2618 added to the other eleven fallbacks, so an input that
reaches this branch but still parses to None no longer logs success alongside
the failure that follows. Narrows this block's bare except at the same time.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018VZ8kYURUMYetyihrfZMEz
@github-actions github-actions Bot added the bug label Aug 28, 2026
Comment thread pr_agent/algo/utils.py
if data is not None:
get_logger().info("Successfully parsed AI prediction after normalizing diff removal markers")
return data
except Exception:
@qodo-code-review

qodo-code-review Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

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

Grey Divider


Remediation recommended

1. except silently suppresses YAML errors 📘 Rule violation ☼ Reliability
Description
The new YAML fallback catches every Exception and handles it only with pass. This violates the
requirement that each caught exception be logged, propagated, translated, or otherwise handled
explicitly.
Code

pr_agent/algo/utils.py[R1097-1098]

+        except Exception:
+            pass
Relevance

●●● Strong

A close YAML fallback precedent accepted replacing silent exception handling with explicit
diagnostic handling.

PR-#2097
PR-#2385
PR-#2862

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2694713 prohibits catch blocks whose only statement is pass; the added handler does exactly
that.

Rule 2694713: Handle all caught exceptions explicitly (no empty catch blocks)
pr_agent/algo/utils.py[1097-1098]

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 diff-marker fallback silently suppresses YAML parsing exceptions with `pass`.

## Issue Context
Compliance rule 2694713 requires every catch block to perform explicit handling such as logging, propagation, or error translation. Preserve the fallback chain while making the failure observable at an appropriate log level.

## Fix Focus Areas
- pr_agent/algo/utils.py[1097-1098]

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


2. Tests use single-quoted literals 📘 Rule violation ⚙ Maintainability
Description
The new regression tests delimit YAML fixtures, dictionary keys, expected values, and call arguments
with single quotes. Double-quoted delimiters can be used without problematic escaping in these
locations.
Code

tests/unittest/test_try_fix_yaml.py[R283-286]

+        expected_output = {
+            'code_suggestions': [
+                {
+                    'relevant_file': 'example.rb\n',
Relevance

●●● Strong

Multiple recent reviews accepted changing newly added test literals to the repository’s double-quote
convention.

PR-#2526
PR-#2569
PR-#2679

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Added test code uses a ''' fixture and single-quoted keys, values, keyword arguments, and inputs,
contrary to rule 2694657.

Rule 2694657: Use double quotes for all Python string literals
tests/unittest/test_try_fix_yaml.py[270-296]
tests/unittest/test_try_fix_yaml.py[304-304]
tests/unittest/test_try_fix_yaml.py[317-317]

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 tests introduce numerous single-quoted Python string literals.

## Issue Context
Compliance rule 2694657 requires double-quoted literals. Keep the literal contents unchanged, including the embedded diff/YAML examples, while changing only Python delimiters.

## Fix Focus Areas
- tests/unittest/test_try_fix_yaml.py[270-296]
- tests/unittest/test_try_fix_yaml.py[304-304]
- tests/unittest/test_try_fix_yaml.py[317-317]

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


3. Inline comment violates E261 📘 Rule violation ✧ Quality
Description
The added continue # real list item has only one space before its inline comment, which triggers
Flake8 E261. Consequently, the modified Python file cannot pass Flake8 with zero errors.
Code

pr_agent/algo/utils.py[1080]

+                continue # real list item → keep as-is
Relevance

●●● Strong

This is a deterministic Flake8 E261 fix, and repository reviews routinely accept concrete lint
corrections.

PR-#2307
PR-#2212

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed line contains continue # real list item → keep as-is; the single space before # is a
known Flake8 E261 violation under the rule requiring a clean Flake8 run.

Rule 2694666: Python code must pass flake8 in CI with zero errors or warnings
pr_agent/algo/utils.py[1080-1080]

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 inline comment has insufficient spacing before `#`, causing Flake8 `E261`.

## Issue Context
PEP 8 and Flake8 require at least two spaces before an inline comment. This is a mechanical formatting fix and must not alter control flow.

## Fix Focus Areas
- pr_agent/algo/utils.py[1080-1080]

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


View medium (3)
4. Leading minus corrupts code 🐞 Bug ≡ Correctness
Description
The new fallback treats an unindented block-scalar line such as -foo as a diff deletion, rewrites
it to  foo, and can then return foo\n as the parsed code, silently dropping the real leading
minus. This is reachable for the insufficiently indented existing_code/improved_code output that
try_fix_yaml is explicitly intended to repair.
Code

pr_agent/algo/utils.py[R1084-1087]

+        while cleaned and cleaned[0] in ('+', '-'):
+            cleaned = cleaned[1:]
+        if cleaned and cleaned[0] not in (' ', '\t'):
+            cleaned = ' ' + cleaned
Relevance

●●● Strong

Recent parser reviews accept focused correctness fixes preventing malformed or invalid input from
being silently misprocessed.

PR-#2755
PR-#2618
PR-#2622

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The general loader invokes try_fix_yaml after initial parsing fails, and the new branch classifies
-foo as a removal marker, strips its hyphen, adds one space, and immediately returns the
now-parseable block scalar. Repository prompts require code-suggestion fields to use YAML block
scalars, while existing tests confirm insufficiently indented code blocks are an expected recovery
case.

pr_agent/algo/utils.py[926-945]
pr_agent/algo/utils.py[1062-1097]
pr_agent/settings/code_suggestions/pr_code_suggestions_reflect_prompts.toml[94-114]
tests/unittest/test_try_fix_yaml.py[239-263]

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 5.5 YAML fallback strips every leading `-`/`+` run and even inserts indentation when the remainder has no whitespace. Consequently, malformed block-scalar code whose actual content starts with a minus, such as `-foo`, becomes `foo` and is returned as a successful parse.

## Issue Context
Diff-wrapped YAML lines in the regression input have whitespace after the marker sequence (for example, `-    example.py` and `-+    print(...)`). Restrict normalization to that provable shape rather than converting arbitrary plain scalars or code beginning with `-`. Add a regression case with an insufficiently indented block scalar containing `-foo` and assert the leading minus is preserved by whichever fallback repairs the indentation.

## Fix Focus Areas
- pr_agent/algo/utils.py[1071-1097]
- tests/unittest/test_try_fix_yaml.py[239-296]

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


5. Fallback uses single-quoted literals 📘 Rule violation ⚙ Maintainability
Description
The new fallback uses single-quoted delimiters for marker, whitespace, and newline literals. These
literals do not need single quotes to avoid escaping, so they violate the required double-quote
style.
Code

pr_agent/algo/utils.py[R1067-1068]

+        if line.startswith('+'):
+            response_text_lines_copy[i] = ' ' + line[1:]
Relevance

●●● Strong

Recent repository reviews consistently accept converting newly added single-quoted literals to
double quotes.

PR-#2836
PR-#2679
PR-#2693

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The fallback uses literals such as '+', ' ', '-', '\t', and '\n', while rule 2694657
requires double-quoted delimiters.

Rule 2694657: Use double quotes for all Python string literals
pr_agent/algo/utils.py[1067-1068]
pr_agent/algo/utils.py[1073-1079]
pr_agent/algo/utils.py[1083-1093]

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 fallback introduces multiple Python string literals delimited with single quotes.

## Issue Context
Compliance rule 2694657 requires double-quoted Python string literals except when single quotes avoid additional escaping; that exception does not apply to these marker and whitespace strings.

## Fix Focus Areas
- pr_agent/algo/utils.py[1067-1093]

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


6. Test prose is non-imperative 📘 Rule violation ⚙ Maintainability
Description
The added test docstring starts with descriptive Ensures, and added comments narrate behavior with
When and This input. The checklist requires behavior-describing docstrings and comments to use
imperative phrasing.
Code

tests/unittest/test_try_fix_yaml.py[R266-269]

+        """
+            Ensures diff-style '-' markers nested inside list items are normalised so the YAML parses
+            into the expected structure.
+        """
Relevance

●● Moderate

Imperative-comment findings are mixed: several recent acceptances exist, but comparable
descriptive-docstring findings were rejected.

PR-#2703
PR-#2817
PR-#2661

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The phrases Ensures..., When all fallbacks..., and This input reaches... are descriptive
rather than imperative, violating rule 2694688.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_try_fix_yaml.py[266-269]
tests/unittest/test_try_fix_yaml.py[298-300]
tests/unittest/test_try_fix_yaml.py[311-313]

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 test documentation and comments use descriptive or narrative phrasing instead of imperative phrasing.

## Issue Context
Rewrite the docstring to begin with an imperative verb such as `Ensure`, and phrase behavior comments as direct instructions while preserving their meaning.

## Fix Focus Areas
- tests/unittest/test_try_fix_yaml.py[266-269]
- tests/unittest/test_try_fix_yaml.py[298-300]
- tests/unittest/test_try_fix_yaml.py[311-313]

ⓘ 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: This is a localized runtime parser change, but its heuristic normalization can alter YAML semantics across inputs, creating real behavioral risk that merits a complete single-pass review.

Grey Divider

Tip of the day
💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

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

Sorted, and merging. I pushed the merge with main onto your branch, added the data is not None guard the other eleven fallbacks got in #2618, and narrowed this block's bare except to clear Qodo's finding. Your commits are untouched.

CI is green again on the new head, for the first time since October. Thanks for the fix and for the long wait on our side.

@IsmaelMartinez
IsmaelMartinez merged commit 39d1e95 into The-PR-Agent:main Aug 28, 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.

4 participants