Skip to content

Fix error about load_yaml when ai response has \\n #1717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dcb1
Copy link

@dcb1 dcb1 commented Apr 23, 2025

User description

when use pr-agent to review code in gitlab by gitlab-runner, met a problem.
The ai model is DeepSeek-V3(update20250324)
image


PR Type

Bug fix


Description

  • Fixes YAML parsing error when AI response contains escaped newlines

  • Replaces all \n strings with actual newlines before parsing


Changes walkthrough 📝

Relevant files
Bug fix
utils.py
Handle escaped newlines in YAML AI response parsing           

pr_agent/algo/utils.py

  • Adds replacement of \n with actual newlines in load_yaml
  • Ensures AI responses with escaped newlines are parsed correctly
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    Copy link
    Contributor

    qodo-merge-pro-for-open-source bot commented Apr 23, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Learned
    best practice
    Add null safety checks before performing string operations to prevent potential runtime errors

    Add a null safety check to ensure response_text is not None before performing
    string operations on it, which would prevent potential AttributeError if
    response_text is None.

    pr_agent/algo/utils.py [708]

    -response_text = response_text.strip('\n').removeprefix('```yaml').rstrip().removesuffix('```').replace('\\n', '\n')
    +if response_text:
    +    response_text = response_text.strip('\n').removeprefix('```yaml').rstrip().removesuffix('```').replace('\\n', '\n')
    +else:
    +    response_text = ""
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    Low
    General
    Fix order of operations

    The replacement of '\n' with '\n' should be done before removing the YAML code
    block markers to ensure proper handling of all escaped newlines in the response
    text.

    pr_agent/algo/utils.py [708]

    -response_text = response_text.strip('\n').removeprefix('```yaml').rstrip().removesuffix('```').replace('\\n', '\n')
    +response_text = response_text.replace('\\n', '\n').strip('\n').removeprefix('```yaml').rstrip().removesuffix('```')
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion correctly identifies that the order of operations could be improved by replacing escaped newlines before removing YAML markers. This would ensure all escaped newlines are properly handled regardless of their position relative to the code block markers. The change is valid but has moderate impact.

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

    @dcb1 dcb1 force-pushed the fix/load_yaml_error branch from d512533 to 89fc8b8 Compare April 23, 2025 05:09
    @mrT23
    Copy link
    Collaborator

    mrT23 commented Apr 23, 2025

    Which model are you using ? Clearly state the model for a PR like this.

    In addition:
    Did the model produce a valid YAML, that can be loaded ? If not, than why are you referring to this as an 'error of load_yaml' ?

    @dcb1 dcb1 marked this pull request as draft April 24, 2025 03:14
    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.

    2 participants