From ddb89a7474e784978ba64454b104548551a63532 Mon Sep 17 00:00:00 2001 From: "Hussam.lawen" Date: Thu, 8 Feb 2024 14:26:14 +0200 Subject: [PATCH 01/12] New PR Reviewer with Table view --- pr_agent/algo/utils.py | 10 ++++-- pr_agent/settings/pr_reviewer_prompts.toml | 37 ++++------------------ 2 files changed, 14 insertions(+), 33 deletions(-) diff --git a/pr_agent/algo/utils.py b/pr_agent/algo/utils.py index 9a150864..44e411b8 100644 --- a/pr_agent/algo/utils.py +++ b/pr_agent/algo/utils.py @@ -52,8 +52,13 @@ def convert_to_markdown(output_data: dict, gfm_supported: bool=True) -> str: if value is None or value == '' or value == {} or value == []: continue if isinstance(value, dict): - markdown_text += f"## {key}\n\n" + if key.lower() == 'pr review': + markdown_text += f"## {key}\n\n" + markdown_text += "\n\n" + markdown_text += """""" + markdown_text += convert_to_markdown(value, gfm_supported) + markdown_text += "\n
     feedback                                          
\n" elif isinstance(value, list): emoji = emojis.get(key, "") if key.lower() == 'code feedback': @@ -82,7 +87,8 @@ def convert_to_markdown(output_data: dict, gfm_supported: bool=True) -> str: else: markdown_text += f"{emoji} **General suggestions:** {value}\n" else: - markdown_text += f"- {emoji} **{key}:** {value}\n" + markdown_text += f" {emoji} {key} {value}\n" + return markdown_text diff --git a/pr_agent/settings/pr_reviewer_prompts.toml b/pr_agent/settings/pr_reviewer_prompts.toml index 736fb247..6705efea 100644 --- a/pr_agent/settings/pr_reviewer_prompts.toml +++ b/pr_agent/settings/pr_reviewer_prompts.toml @@ -46,21 +46,7 @@ Extra instructions from the user: You must use the following YAML schema to format your answer: ```yaml -PR Analysis: - Main theme: - type: string - description: a short explanation of the PR - PR summary: - type: string - description: summary of the PR in 2-3 sentences. - Type of PR: - type: string - enum: - - Bug fix - - Tests - - Enhancement - - Documentation - - Other +PR Review: {%- if require_score %} Score: type: int @@ -98,14 +84,9 @@ PR Analysis: Take into account the size, complexity, quality, and the needed changes of the PR code diff. Explain your answer shortly (1-2 sentences). Use the format: '1, because ...' {%- endif %} +{%- if num_code_suggestions > 0 or require_security %} PR Feedback: - General suggestions: - type: string - description: |- - General suggestions and feedback for the contributors and maintainers of this PR. - May include important suggestions for the overall structure, - primary purpose, best practices, critical bugs, and other aspects of the PR. - Don't address PR title and description, or lack of tests. Explain your suggestions. +{% endif %} {%- if num_code_suggestions > 0 %} Code feedback: type: array @@ -140,13 +121,7 @@ PR Feedback: Example output: ```yaml -PR Analysis: - Main theme: |- - xxx - PR summary: |- - xxx - Type of PR: |- - ... +PR Review: {%- if require_score %} Score: 89 {%- endif %} @@ -159,9 +134,9 @@ PR Analysis: Estimated effort to review [1-5]: |- 3, because ... {%- endif %} +{%- if num_code_suggestions > 0 or require_security %} PR Feedback: - General PR suggestions: |- - ... +{% endif %} {%- if num_code_suggestions > 0 %} Code feedback: - relevant file: |- From 4447110118aeb88544c11fc988c603e0b4900183 Mon Sep 17 00:00:00 2001 From: "Hussam.lawen" Date: Thu, 8 Feb 2024 14:35:39 +0200 Subject: [PATCH 02/12] small fix --- pr_agent/algo/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pr_agent/algo/utils.py b/pr_agent/algo/utils.py index b400354c..b4d05324 100644 --- a/pr_agent/algo/utils.py +++ b/pr_agent/algo/utils.py @@ -60,7 +60,7 @@ def convert_to_markdown(output_data: dict, gfm_supported: bool=True) -> str: if key.lower() == 'pr review': markdown_text += f"## {key}\n\n" markdown_text += "\n\n" - markdown_text += """""" + markdown_text += """""" markdown_text += convert_to_markdown(value, gfm_supported) markdown_text += "\n
     feedback                                          
     Feedback                                          
\n" From a3f4c446320ddc0c9804e3f1247b64ae458bf999 Mon Sep 17 00:00:00 2001 From: "Hussam.lawen" Date: Thu, 8 Feb 2024 15:25:43 +0200 Subject: [PATCH 03/12] PR Review --- pr_agent/tools/pr_reviewer.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pr_agent/tools/pr_reviewer.py b/pr_agent/tools/pr_reviewer.py index e714b194..c402b33e 100644 --- a/pr_agent/tools/pr_reviewer.py +++ b/pr_agent/tools/pr_reviewer.py @@ -128,7 +128,7 @@ class PRReviewer: # publish the review if get_settings().pr_reviewer.persistent_comment and not self.incremental.is_incremental: self.git_provider.publish_persistent_comment(pr_comment, - initial_header="## PR Analysis", + initial_header="## PR Review", update_header=True) else: self.git_provider.publish_comment(pr_comment) @@ -198,9 +198,9 @@ class PRReviewer: if security_concerns is not None: del pr_feedback['Security concerns'] if type(security_concerns) == bool and security_concerns == False: - data.setdefault('PR Analysis', {})['Security concerns'] = 'No security concerns found' + data.setdefault('PR Review', {})['Security concerns'] = 'No security concerns found' else: - data.setdefault('PR Analysis', {})['Security concerns'] = security_concerns + data.setdefault('PR Review', {})['Security concerns'] = security_concerns # if 'Code feedback' in pr_feedback: @@ -376,12 +376,12 @@ class PRReviewer: try: review_labels = [] if get_settings().pr_reviewer.enable_review_labels_effort: - estimated_effort = data['PR Analysis']['Estimated effort to review [1-5]'] + estimated_effort = data['PR Review']['Estimated effort to review [1-5]'] estimated_effort_number = int(estimated_effort.split(',')[0]) if 1 <= estimated_effort_number <= 5: # 1, because ... review_labels.append(f'Review effort [1-5]: {estimated_effort_number}') if get_settings().pr_reviewer.enable_review_labels_security: - security_concerns = data['PR Analysis']['Security concerns'] # yes, because ... + security_concerns = data['PR Review']['Security concerns'] # yes, because ... security_concerns_bool = 'yes' in security_concerns.lower() or 'true' in security_concerns.lower() if security_concerns_bool: review_labels.append('Possible security concern') From fa077dc516298acb665fb084b4296ffc5ebea1f1 Mon Sep 17 00:00:00 2001 From: mrT23 Date: Thu, 8 Feb 2024 17:08:42 +0200 Subject: [PATCH 04/12] formatting --- docs/REVIEW.md | 1 - pr_agent/algo/utils.py | 96 +++++------- pr_agent/git_providers/bitbucket_provider.py | 4 +- .../bitbucket_server_provider.py | 4 +- pr_agent/git_providers/github_provider.py | 4 +- pr_agent/git_providers/gitlab_provider.py | 4 +- pr_agent/settings/configuration.toml | 4 +- pr_agent/settings/pr_description_prompts.toml | 2 +- pr_agent/settings/pr_reviewer_prompts.toml | 144 ++++++------------ pr_agent/tools/pr_reviewer.py | 45 +++--- tests/unittest/test_convert_to_markdown.py | 2 - tests/unittest/test_load_yaml.py | 4 +- 12 files changed, 115 insertions(+), 199 deletions(-) diff --git a/docs/REVIEW.md b/docs/REVIEW.md index 19c4880b..ae2d3876 100644 --- a/docs/REVIEW.md +++ b/docs/REVIEW.md @@ -42,7 +42,6 @@ To edit [configurations](./../pr_agent/settings/configuration.toml#L19) related - `require_focused_review`: if set to true, the tool will add a section - 'is the PR a focused one'. Default is false. - `require_score_review`: if set to true, the tool will add a section that scores the PR. Default is false. - `require_tests_review`: if set to true, the tool will add a section that checks if the PR contains tests. Default is true. -- `require_security_review`: if set to true, the tool will add a section that checks if the PR contains security issues. Default is true. - `require_estimate_effort_to_review`: if set to true, the tool will add a section that estimates the effort needed to review the PR. Default is true. #### SOC2 ticket compliance ๐Ÿ’Ž This sub-tool checks if the PR description properly contains a ticket to a project management system (e.g., Jira, Asana, Trello, etc.), as required by SOC2 compliance. If not, it will add a label to the PR: "Missing SOC2 ticket". diff --git a/pr_agent/algo/utils.py b/pr_agent/algo/utils.py index b4d05324..575d1175 100644 --- a/pr_agent/algo/utils.py +++ b/pr_agent/algo/utils.py @@ -36,15 +36,11 @@ def convert_to_markdown(output_data: dict, gfm_supported: bool=True) -> str: Returns: str: The markdown formatted text generated from the input dictionary. """ - markdown_text = "" emojis = { - "Main theme": "๐ŸŽฏ", - "PR summary": "๐Ÿ“", - "Type of PR": "๐Ÿ“Œ", + "Possible issues": "๐Ÿ”", "Score": "๐Ÿ…", "Relevant tests added": "๐Ÿงช", - "Unrelated changes": "โš ๏ธ", "Focused PR": "โœจ", "Security concerns": "๐Ÿ”’", "General suggestions": "๐Ÿ’ก", @@ -52,76 +48,62 @@ def convert_to_markdown(output_data: dict, gfm_supported: bool=True) -> str: "Code feedback": "๐Ÿค–", "Estimated effort to review [1-5]": "โฑ๏ธ", } - - for key, value in output_data.items(): + markdown_text = "" + markdown_text += f"## PR Review\n\n" + markdown_text += "\n\n" + markdown_text += """""" + for key, value in output_data['review'].items(): if value is None or value == '' or value == {} or value == []: continue - if isinstance(value, dict): - if key.lower() == 'pr review': - markdown_text += f"## {key}\n\n" - markdown_text += "
     PR feedback                    
\n\n" - markdown_text += """""" + key_nice = key.replace('_', ' ').capitalize() + emoji = emojis.get(key_nice, "") + markdown_text += f"\n" + markdown_text += "
     Feedback                                          
{emoji} {key_nice}\n\n{value}\n\n
\n" + + if 'code_feedback' in output_data: + if gfm_supported: + markdown_text += f"\n\n" + markdown_text += f"
Code feedback:\n\n" + else: + markdown_text += f"\n\n** Code feedback:**\n\n" + markdown_text += "
" + for i, value in enumerate(output_data['code_feedback']): + if value is None or value == '' or value == {} or value == []: + continue + markdown_text += parse_code_suggestion(value, i, gfm_supported)+"\n\n" + if markdown_text.endswith('
'): + markdown_text = markdown_text[:-4] + if gfm_supported: + markdown_text += f"
" + #print(markdown_text) - markdown_text += convert_to_markdown(value, gfm_supported) - markdown_text += "\n\n" - elif isinstance(value, list): - emoji = emojis.get(key, "") - if key.lower() == 'code feedback': - if gfm_supported: - markdown_text += f"\n\n" - markdown_text += f"
{ emoji } Code feedback:" - else: - markdown_text += f"\n\n**{emoji} Code feedback:**\n\n" - else: - markdown_text += f"- {emoji} **{key}:**\n\n" - for i, item in enumerate(value): - if isinstance(item, dict) and key.lower() == 'code feedback': - markdown_text += parse_code_suggestion(item, i, gfm_supported) - elif item: - markdown_text += f" - {item}\n" - if key.lower() == 'code feedback': - if gfm_supported: - markdown_text += "
\n\n" - else: - markdown_text += "\n\n" - elif value != 'n/a': - emoji = emojis.get(key, "") - if key.lower() == 'general suggestions': - if gfm_supported: - markdown_text += f"\n\n{emoji} General suggestions: {value}\n" - else: - markdown_text += f"{emoji} **General suggestions:** {value}\n" - else: - markdown_text += f" {emoji} {key} {value}\n" return markdown_text -def parse_code_suggestion(code_suggestions: dict, i: int = 0, gfm_supported: bool = True) -> str: +def parse_code_suggestion(code_suggestion: dict, i: int = 0, gfm_supported: bool = True) -> str: """ Convert a dictionary of data into markdown format. Args: - code_suggestions (dict): A dictionary containing data to be converted to markdown format. + code_suggestion (dict): A dictionary containing data to be converted to markdown format. Returns: str: A string containing the markdown formatted text generated from the input dictionary. """ markdown_text = "" - if gfm_supported and 'relevant line' in code_suggestions: - if i == 0: - markdown_text += "
" + if gfm_supported and 'relevant_line' in code_suggestion: markdown_text += '' - for sub_key, sub_value in code_suggestions.items(): + for sub_key, sub_value in code_suggestion.items(): try: - if sub_key.lower() == 'relevant file': + if sub_key.lower() == 'relevant_file': relevant_file = sub_value.strip('`').strip('"').strip("'") - markdown_text += f"" + markdown_text += f"" # continue elif sub_key.lower() == 'suggestion': markdown_text += (f"" - f"") - elif sub_key.lower() == 'relevant line': + f"") + elif sub_key.lower() == 'relevant_line': markdown_text += f"" sub_value_list = sub_value.split('](') relevant_line = sub_value_list[0].lstrip('`').lstrip('[') @@ -137,7 +119,7 @@ def parse_code_suggestion(code_suggestions: dict, i: int = 0, gfm_supported: boo markdown_text += '
{sub_key}{relevant_file}
relevant file{relevant_file}
{sub_key}      
\n\n**{sub_value.strip()}**\n
\n\n\n\n{sub_value.strip()}\n\n\n
relevant line
' markdown_text += "
" else: - for sub_key, sub_value in code_suggestions.items(): + for sub_key, sub_value in code_suggestion.items(): if isinstance(sub_value, dict): # "code example" markdown_text += f" - **{sub_key}:**\n" for code_key, code_value in sub_value.items(): # 'before' and 'after' code @@ -145,12 +127,12 @@ def parse_code_suggestion(code_suggestions: dict, i: int = 0, gfm_supported: boo code_str_indented = textwrap.indent(code_str, ' ') markdown_text += f" - **{code_key}:**\n{code_str_indented}\n" else: - if "relevant file" in sub_key.lower(): + if "relevant_file" in sub_key.lower(): markdown_text += f"\n - **{sub_key}:** {sub_value} \n" else: markdown_text += f" **{sub_key}:** {sub_value} \n" if not gfm_supported: - if "relevant line" not in sub_key.lower(): # nicer presentation + if "relevant_line" not in sub_key.lower(): # nicer presentation # markdown_text = markdown_text.rstrip('\n') + "\\\n" # works for gitlab markdown_text = markdown_text.rstrip('\n') + " \n" # works for gitlab and bitbucker @@ -347,7 +329,7 @@ def load_yaml(response_text: str, keys_fix_yaml: List[str] = []) -> dict: def try_fix_yaml(response_text: str, keys_fix_yaml: List[str] = []) -> dict: response_text_lines = response_text.split('\n') - keys = ['relevant line:', 'suggestion content:', 'relevant file:', 'existing code:', 'improved code:'] + keys = ['relevant line:', 'suggestion content:', 'relevant_file:', 'existing code:', 'improved code:'] keys = keys + keys_fix_yaml # first fallback - try to convert 'relevant line: ...' to relevant line: |-\n ...' response_text_lines_copy = response_text_lines.copy() diff --git a/pr_agent/git_providers/bitbucket_provider.py b/pr_agent/git_providers/bitbucket_provider.py index c761d10b..6d701ad4 100644 --- a/pr_agent/git_providers/bitbucket_provider.py +++ b/pr_agent/git_providers/bitbucket_provider.py @@ -240,8 +240,8 @@ class BitbucketProvider(GitProvider): def generate_link_to_relevant_line_number(self, suggestion) -> str: try: - relevant_file = suggestion['relevant file'].strip('`').strip("'") - relevant_line_str = suggestion['relevant line'] + relevant_file = suggestion['relevant_file'].strip('`').strip("'") + relevant_line_str = suggestion['relevant_line'] if not relevant_line_str: return "" diff --git a/pr_agent/git_providers/bitbucket_server_provider.py b/pr_agent/git_providers/bitbucket_server_provider.py index 9798cd5e..df1492dd 100644 --- a/pr_agent/git_providers/bitbucket_server_provider.py +++ b/pr_agent/git_providers/bitbucket_server_provider.py @@ -246,8 +246,8 @@ class BitbucketServerProvider(GitProvider): def generate_link_to_relevant_line_number(self, suggestion) -> str: try: - relevant_file = suggestion['relevant file'].strip('`').strip("'") - relevant_line_str = suggestion['relevant line'] + relevant_file = suggestion['relevant_file'].strip('`').strip("'") + relevant_line_str = suggestion['relevant_line'] if not relevant_line_str: return "" diff --git a/pr_agent/git_providers/github_provider.py b/pr_agent/git_providers/github_provider.py index 8dd8a87f..f5ec7129 100644 --- a/pr_agent/git_providers/github_provider.py +++ b/pr_agent/git_providers/github_provider.py @@ -603,8 +603,8 @@ class GithubProvider(GitProvider): def generate_link_to_relevant_line_number(self, suggestion) -> str: try: - relevant_file = suggestion['relevant file'].strip('`').strip("'") - relevant_line_str = suggestion['relevant line'] + relevant_file = suggestion['relevant_file'].strip('`').strip("'").strip('\n') + relevant_line_str = suggestion['relevant_line'].strip('\n') if not relevant_line_str: return "" diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index 85525e6c..889776af 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -450,8 +450,8 @@ class GitLabProvider(GitProvider): def generate_link_to_relevant_line_number(self, suggestion) -> str: try: - relevant_file = suggestion['relevant file'].strip('`').strip("'") - relevant_line_str = suggestion['relevant line'] + relevant_file = suggestion['relevant_file'].strip('`').strip("'") + relevant_line_str = suggestion['relevant_line'] if not relevant_line_str: return "" diff --git a/pr_agent/settings/configuration.toml b/pr_agent/settings/configuration.toml index 7555c292..49e1880b 100644 --- a/pr_agent/settings/configuration.toml +++ b/pr_agent/settings/configuration.toml @@ -9,7 +9,7 @@ verbosity_level=0 # 0,1,2 use_extra_bad_extensions=false use_repo_settings_file=true use_global_settings_file=true -ai_timeout=90 +ai_timeout=120 # 2minutes max_description_tokens = 500 max_commits_tokens = 500 max_model_tokens = 32000 # Limits the maximum number of tokens that can be used by any model, regardless of the model's default capabilities. @@ -22,7 +22,6 @@ cli_mode=false require_focused_review=false require_score_review=false require_tests_review=true -require_security_review=true require_estimate_effort_to_review=true # soc2 require_soc2_ticket=false @@ -149,7 +148,6 @@ push_commands = [ --pr_reviewer.require_focused_review=false \ --pr_reviewer.require_score_review=false \ --pr_reviewer.require_tests_review=false \ - --pr_reviewer.require_security_review=false \ --pr_reviewer.require_estimate_effort_to_review=false \ --pr_reviewer.num_code_suggestions=0 \ --pr_reviewer.inline_code_comments=false \ diff --git a/pr_agent/settings/pr_description_prompts.toml b/pr_agent/settings/pr_description_prompts.toml index b36b0183..80df6d60 100644 --- a/pr_agent/settings/pr_description_prompts.toml +++ b/pr_agent/settings/pr_description_prompts.toml @@ -91,7 +91,7 @@ labels: {%- endif %} ``` -Answer should be a valid YAML, and nothing else. Each YAML output MUST be after a newline, with proper indent, and block scalar indicator ('|-') +Answer should be a valid YAML, and nothing else. Each YAML output MUST be after a newline, with proper indent, and block scalar indicator ('|') """ user="""PR Info: diff --git a/pr_agent/settings/pr_reviewer_prompts.toml b/pr_agent/settings/pr_reviewer_prompts.toml index adc72172..01b293b4 100644 --- a/pr_agent/settings/pr_reviewer_prompts.toml +++ b/pr_agent/settings/pr_reviewer_prompts.toml @@ -43,121 +43,71 @@ Extra instructions from the user: {% endif %} -You must use the following YAML schema to format your answer: -```yaml -PR Review: +The output must be a YAML object equivalent to type $PRReview, according to the following Pydantic definitions: +===== +class Review(BaseModel) +{%- if require_estimate_effort_to_review %} + estimated_effort_to_review_[1-5]: str = Field(description="Estimate, on a scale of 1-5 (inclusive), the time and effort required to review this PR by an experienced and knowledgeable developer. 1 means short and easy review , 5 means long and hard review. Take into account the size, complexity, quality, and the needed changes of the PR code diff. Explain your answer in a short and concise manner.") +{%- endif %} {%- if require_score %} - Score: - type: int - description: |- - Rate this PR on a scale of 0-100 (inclusive), where 0 means the worst - possible PR code, and 100 means PR code of the highest quality, without - any bugs or performance issues, that is ready to be merged immediately and - run in production at scale. + score: str = Field(description="Rate this PR on a scale of 0-100 (inclusive), where 0 means the worst possible PR code, and 100 means PR code of the highest quality, without any bugs or performance issues, that is ready to be merged immediately and run in production at scale.") {%- endif %} {%- if require_tests %} - Relevant tests added: - type: string - description: yes\\no question: does this PR have relevant tests ? + relevant_tests_added: str = Field(description="yes\\no question: does this PR have relevant tests ?") {%- endif %} {%- if question_str %} - Insights from user's answer: - type: string - description: |- - shortly summarize the insights you gained from the user's answers to the questions + insights_from_user_answers: str = Field(description="shortly summarize the insights you gained from the user's answers to the questions") {%- endif %} {%- if require_focused %} - Focused PR: - type: string - description: |- - Is this a focused PR, in the sense that all the PR code diff changes are - united under a single focused theme ? If the theme is too broad, or the PR - code diff changes are too scattered, then the PR is not focused. Explain - your answer shortly. + focused_pr: str = Field(description="Is this a focused PR, in the sense that all the PR code diff changes are united under a single focused theme ? If the theme is too broad, or the PR code diff changes are too scattered, then the PR is not focused. Explain your answer shortly.") {%- endif %} -{%- if require_estimate_effort_to_review %} - Estimated effort to review [1-5]: - type: string - description: >- - Estimate, on a scale of 1-5 (inclusive), the time and effort required to review this PR by an experienced and knowledgeable developer. 1 means short and easy review , 5 means long and hard review. - Take into account the size, complexity, quality, and the needed changes of the PR code diff. - Explain your answer shortly (1-2 sentences). Use the format: '1, because ...' -{%- endif %} -{%- if num_code_suggestions > 0 or require_security %} -PR Feedback: -{% endif %} -{%- if num_code_suggestions > 0 %} - Code feedback: - type: array - maxItems: {{ num_code_suggestions }} - uniqueItems: true - items: - relevant file: - type: string - description: the relevant file full path - language: - type: string - description: the language of the relevant file - suggestion: - type: string - description: |- - a concrete suggestion for meaningfully improving the new PR code. - Also describe how, specifically, the suggestion can be applied to new PR code. - Add tags with importance measure that matches each suggestion ('important' or 'medium'). - Do not make suggestions for updating or adding docstrings, renaming PR title and description, or linter like. - relevant line: - type: string - description: |- - a single code line taken from the relevant file, to which the suggestion applies. - The code line should start with a '+'. - Make sure to output the line exactly as it appears in the relevant file -{%- endif %} -{%- if require_security %} - Security concerns: - type: string - description: >- - does this PR code introduce possible vulnerabilities such as exposure of sensitive information (e.g., API keys, secrets, passwords), or security concerns like SQL injection, XSS, CSRF, and others ? Answer 'No' if there are no possible issues. - Answer 'Yes, because ...' if there are security concerns or issues. Explain your answer shortly. -{%- endif %} -``` + possible_issues: str = Field(description="Does this PR code introduce clear issues, bugs, or performance concerns ? Answer 'No' if there are no clear issues. Answer 'Yes, ...' if there are issues, and explain your answer shortly.") + security_concerns: str = Field(description="does this PR code introduce possible vulnerabilities such as exposure of sensitive information (e.g., API keys, secrets, passwords), or security concerns like SQL injection, XSS, CSRF, and others ? Answer 'No' if there are no possible issues. Answer 'Yes, because ...' if there are security concerns or issues. Explain your answer shortly.") + +class CodeSuggestion(BaseModel) + relevant_file: str = Field(description="the relevant file full path") + language: str = Field(description="the language of the relevant file") + suggestion: str = Field(description="a concrete suggestion for meaningfully improving the new PR code. Also describe how, specifically, the suggestion can be applied to new PR code. Add tags with importance measure that matches each suggestion ('important' or 'medium'). Do not make suggestions for updating or adding docstrings, renaming PR title and description, or linter like.") + relevant_line: str = Field(description="a single code line taken from the relevant file, to which the suggestion applies. The code line should start with a '+'. Make sure to output the line exactly as it appears in the relevant file") + +class PRReview(BaseModel) + review: Review + code_feedback: List[CodeSuggestion] Example output: ```yaml -PR Review: -{%- if require_score %} - Score: 89 -{%- endif %} - Relevant tests added: |- - No -{%- if require_focused %} - Focused PR: no, because ... -{%- endif %} +review: {%- if require_estimate_effort_to_review %} - Estimated effort to review [1-5]: |- + estimated_effort_to_review_[1-5]: | 3, because ... {%- endif %} -{%- if num_code_suggestions > 0 or require_security %} -PR Feedback: -{% endif %} -{%- if num_code_suggestions > 0 %} - Code feedback: - - relevant file: |- - directory/xxx.py - language: |- - python - suggestion: |- - xxx [important] - relevant line: |- - xxx - ... +{%- if require_score %} + score: 89 {%- endif %} -{%- if require_security %} - Security concerns: No + relevant_tests_added: | + No +{%- if require_focused %} + focused_pr: | + no, because ... +{%- endif %} + possible_issues: | + No + security_concerns: | + No +{%- if num_code_suggestions > 0 %} +code_feedback +- relevant_file: | + directory/xxx.py + language: | + python + suggestion: | + xxx [important] + relevant_line: | + xxx {%- endif %} ``` -Each YAML output MUST be after a newline, indented, with block scalar indicator ('|-'). -Don't repeat the prompt in the answer, and avoid outputting the 'type' and 'description' fields. +Answer should be a valid YAML, and nothing else. Each YAML output MUST be after a newline, with proper indent, and block scalar indicator ('|') """ user="""PR Info: diff --git a/pr_agent/tools/pr_reviewer.py b/pr_agent/tools/pr_reviewer.py index c402b33e..f6318a31 100644 --- a/pr_agent/tools/pr_reviewer.py +++ b/pr_agent/tools/pr_reviewer.py @@ -12,7 +12,8 @@ from pr_agent.algo.ai_handlers.base_ai_handler import BaseAiHandler from pr_agent.algo.ai_handlers.litellm_ai_handler import LiteLLMAIHandler from pr_agent.algo.pr_processing import get_pr_diff, retry_with_fallback_models from pr_agent.algo.token_handler import TokenHandler -from pr_agent.algo.utils import convert_to_markdown, load_yaml, try_fix_yaml, set_custom_labels, get_user_labels +from pr_agent.algo.utils import convert_to_markdown, load_yaml, try_fix_yaml, set_custom_labels, get_user_labels, \ + ModelType from pr_agent.config_loader import get_settings from pr_agent.git_providers import get_git_provider from pr_agent.git_providers.git_provider import IncrementalPR, get_main_pr_language @@ -62,7 +63,6 @@ class PRReviewer: "diff": "", # empty diff for initial calculation "require_score": get_settings().pr_reviewer.require_score_review, "require_tests": get_settings().pr_reviewer.require_tests_review, - "require_security": get_settings().pr_reviewer.require_security_review, "require_focused": get_settings().pr_reviewer.require_focused_review, "require_estimate_effort_to_review": get_settings().pr_reviewer.require_estimate_effort_to_review, 'num_code_suggestions': get_settings().pr_reviewer.num_code_suggestions, @@ -113,7 +113,7 @@ class PRReviewer: if get_settings().config.publish_output: self.git_provider.publish_comment("Preparing review...", is_temporary=True) - await retry_with_fallback_models(self._prepare_prediction) + await retry_with_fallback_models(self._prepare_prediction, model_type=ModelType.TURBO) if not self.prediction: self.git_provider.remove_initial_comment() return None @@ -192,41 +192,30 @@ class PRReviewer: """ data = load_yaml(self.prediction.strip()) - # Move 'Security concerns' key to 'PR Analysis' section for better display - pr_feedback = data.get('PR Feedback', {}) - security_concerns = pr_feedback.get('Security concerns') - if security_concerns is not None: - del pr_feedback['Security concerns'] - if type(security_concerns) == bool and security_concerns == False: - data.setdefault('PR Review', {})['Security concerns'] = 'No security concerns found' - else: - data.setdefault('PR Review', {})['Security concerns'] = security_concerns - - # - if 'Code feedback' in pr_feedback: - code_feedback = pr_feedback['Code feedback'] + if 'code_feedback' in data: + code_feedback = data['code_feedback'] # Filter out code suggestions that can be submitted as inline comments if get_settings().pr_reviewer.inline_code_comments: - del pr_feedback['Code feedback'] + del data['code_feedback'] else: for suggestion in code_feedback: - if ('relevant file' in suggestion) and (not suggestion['relevant file'].startswith('``')): - suggestion['relevant file'] = f"``{suggestion['relevant file']}``" + if ('relevant_file' in suggestion) and (not suggestion['relevant_file'].startswith('``')): + suggestion['relevant_file'] = f"``{suggestion['relevant_file']}``" - if 'relevant line' not in suggestion: - suggestion['relevant line'] = '' + if 'relevant_line' not in suggestion: + suggestion['relevant_line'] = '' - relevant_line_str = suggestion['relevant line'].split('\n')[0] + relevant_line_str = suggestion['relevant_line'].split('\n')[0] # removing '+' - suggestion['relevant line'] = relevant_line_str.lstrip('+').strip() + suggestion['relevant_line'] = relevant_line_str.lstrip('+').strip() # try to add line numbers link to code suggestions if hasattr(self.git_provider, 'generate_link_to_relevant_line_number'): link = self.git_provider.generate_link_to_relevant_line_number(suggestion) if link: - suggestion['relevant line'] = f"[{suggestion['relevant line']}]({link})" + suggestion['relevant_line'] = f"[{suggestion['relevant_line']}]({link})" else: pass @@ -275,8 +264,8 @@ class PRReviewer: data = load_yaml(self.prediction.strip()) comments: List[str] = [] for suggestion in data.get('PR Feedback', {}).get('Code feedback', []): - relevant_file = suggestion.get('relevant file', '').strip() - relevant_line_in_file = suggestion.get('relevant line', '').strip() + relevant_file = suggestion.get('relevant_file', '').strip() + relevant_line_in_file = suggestion.get('relevant_line', '').strip() content = suggestion.get('suggestion', '') if not relevant_file or not relevant_line_in_file or not content: get_logger().info("Skipping inline comment with missing file/line/content") @@ -376,12 +365,12 @@ class PRReviewer: try: review_labels = [] if get_settings().pr_reviewer.enable_review_labels_effort: - estimated_effort = data['PR Review']['Estimated effort to review [1-5]'] + estimated_effort = data['review']['estimated_effort_to_review_[1-5]'] estimated_effort_number = int(estimated_effort.split(',')[0]) if 1 <= estimated_effort_number <= 5: # 1, because ... review_labels.append(f'Review effort [1-5]: {estimated_effort_number}') if get_settings().pr_reviewer.enable_review_labels_security: - security_concerns = data['PR Review']['Security concerns'] # yes, because ... + security_concerns = data['review']['security_concerns'] # yes, because ... security_concerns_bool = 'yes' in security_concerns.lower() or 'true' in security_concerns.lower() if security_concerns_bool: review_labels.append('Possible security concern') diff --git a/tests/unittest/test_convert_to_markdown.py b/tests/unittest/test_convert_to_markdown.py index 36b0371d..954cee66 100644 --- a/tests/unittest/test_convert_to_markdown.py +++ b/tests/unittest/test_convert_to_markdown.py @@ -49,7 +49,6 @@ class TestConvertToMarkdown: 'Main theme': 'Test', 'Type of PR': 'Test type', 'Relevant tests added': 'no', - 'Unrelated changes': 'n/a', # won't be included in the output 'Focused PR': 'Yes', 'General PR suggestions': 'general suggestion...', 'Code feedback': [ @@ -87,7 +86,6 @@ class TestConvertToMarkdown: 'Main theme': {}, 'Type of PR': {}, 'Relevant tests added': {}, - 'Unrelated changes': {}, 'Focused PR': {}, 'General PR suggestions': {}, 'Code suggestions': {} diff --git a/tests/unittest/test_load_yaml.py b/tests/unittest/test_load_yaml.py index a77c847b..4cf436bf 100644 --- a/tests/unittest/test_load_yaml.py +++ b/tests/unittest/test_load_yaml.py @@ -34,7 +34,7 @@ PR Feedback: with pytest.raises(ScannerError): yaml.safe_load(yaml_str) - expected_output = {'PR Analysis': {'Main theme': 'Enhancing the `/describe` command prompt by adding title and description', 'Type of PR': 'Enhancement', 'Relevant tests added': False, 'Focused PR': 'Yes, the PR is focused on enhancing the `/describe` command prompt.'}, 'PR Feedback': {'General suggestions': 'The PR seems to be well-structured and focused on a specific enhancement. However, it would be beneficial to add tests to ensure the new feature works as expected.', 'Code feedback': [{'relevant file': 'pr_agent/settings/pr_description_prompts.toml', 'suggestion': "Consider using a more descriptive variable name than 'user' for the command prompt. A more descriptive name would make the code more readable and maintainable. [medium]", 'relevant line': 'user="""PR Info: aaa'}], 'Security concerns': False}} + expected_output = {'PR Analysis': {'Main theme': 'Enhancing the `/describe` command prompt by adding title and description', 'Type of PR': 'Enhancement', 'Relevant tests added': False, 'Focused PR': 'Yes, the PR is focused on enhancing the `/describe` command prompt.'}, 'PR Feedback': {'General suggestions': 'The PR seems to be well-structured and focused on a specific enhancement. However, it would be beneficial to add tests to ensure the new feature works as expected.', 'Code feedback': [{'relevant_file': 'pr_agent/settings/pr_description_prompts.toml', 'suggestion': "Consider using a more descriptive variable name than 'user' for the command prompt. A more descriptive name would make the code more readable and maintainable. [medium]", 'relevant_line': 'user="""PR Info: aaa'}], 'Security concerns': False}} assert load_yaml(yaml_str) == expected_output def test_load_invalid_yaml2(self): @@ -45,7 +45,7 @@ PR Feedback: with pytest.raises(ScannerError): yaml.safe_load(yaml_str) - expected_output =[{'relevant file': 'src/app.py:', + expected_output =[{'relevant_file': 'src/app.py:', 'suggestion content': 'The print statement is outside inside the if __name__ ==: '}] assert load_yaml(yaml_str) == expected_output From 2e1462580fd61611dbb607cb39b2882070c03eec Mon Sep 17 00:00:00 2001 From: mrT23 Date: Thu, 8 Feb 2024 19:02:56 +0200 Subject: [PATCH 05/12] s --- pr_agent/settings/pr_reviewer_prompts.toml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/pr_agent/settings/pr_reviewer_prompts.toml b/pr_agent/settings/pr_reviewer_prompts.toml index 01b293b4..dd548b03 100644 --- a/pr_agent/settings/pr_reviewer_prompts.toml +++ b/pr_agent/settings/pr_reviewer_prompts.toml @@ -1,6 +1,10 @@ [pr_review_prompt] system="""You are PR-Reviewer, a language model designed to review a Git Pull Request (PR). +{%- if num_code_suggestions > 0 %} Your task is to provide constructive and concise feedback for the PR, and also provide meaningful code suggestions. +{%- else %} +Your task is to provide constructive and concise feedback for the PR. +{%- endif %} The review should focus on new code added in the PR diff (lines starting with '+') Example PR Diff: @@ -64,15 +68,26 @@ class Review(BaseModel) possible_issues: str = Field(description="Does this PR code introduce clear issues, bugs, or performance concerns ? Answer 'No' if there are no clear issues. Answer 'Yes, ...' if there are issues, and explain your answer shortly.") security_concerns: str = Field(description="does this PR code introduce possible vulnerabilities such as exposure of sensitive information (e.g., API keys, secrets, passwords), or security concerns like SQL injection, XSS, CSRF, and others ? Answer 'No' if there are no possible issues. Answer 'Yes, because ...' if there are security concerns or issues. Explain your answer shortly.") +{%- if num_code_suggestions > 0 %} + class CodeSuggestion(BaseModel) relevant_file: str = Field(description="the relevant file full path") language: str = Field(description="the language of the relevant file") suggestion: str = Field(description="a concrete suggestion for meaningfully improving the new PR code. Also describe how, specifically, the suggestion can be applied to new PR code. Add tags with importance measure that matches each suggestion ('important' or 'medium'). Do not make suggestions for updating or adding docstrings, renaming PR title and description, or linter like.") relevant_line: str = Field(description="a single code line taken from the relevant file, to which the suggestion applies. The code line should start with a '+'. Make sure to output the line exactly as it appears in the relevant file") +{%- endif %} +{%- if num_code_suggestions > 0 %} class PRReview(BaseModel) review: Review code_feedback: List[CodeSuggestion] +{%- else %} + +class PRReview(BaseModel) + review: Review +{%- endif %} +===== + Example output: ```yaml From 77e7463395acb4e8a756b59d69eea5f3b5ace8d4 Mon Sep 17 00:00:00 2001 From: "Hussam.lawen" Date: Thu, 8 Feb 2024 20:14:25 +0200 Subject: [PATCH 06/12] fix tests --- pr_agent/algo/utils.py | 4 ++ tests/unittest/test_convert_to_markdown.py | 62 ++++++++-------------- 2 files changed, 25 insertions(+), 41 deletions(-) diff --git a/pr_agent/algo/utils.py b/pr_agent/algo/utils.py index 575d1175..7b189fca 100644 --- a/pr_agent/algo/utils.py +++ b/pr_agent/algo/utils.py @@ -52,6 +52,10 @@ def convert_to_markdown(output_data: dict, gfm_supported: bool=True) -> str: markdown_text += f"## PR Review\n\n" markdown_text += "\n\n" markdown_text += """""" + + if not output_data or not output_data.get('review', {}): + return "" + for key, value in output_data['review'].items(): if value is None or value == '' or value == {} or value == []: continue diff --git a/tests/unittest/test_convert_to_markdown.py b/tests/unittest/test_convert_to_markdown.py index 954cee66..bbc0ace1 100644 --- a/tests/unittest/test_convert_to_markdown.py +++ b/tests/unittest/test_convert_to_markdown.py @@ -45,54 +45,33 @@ Additional aspects: class TestConvertToMarkdown: # Tests that the function works correctly with a simple dictionary input def test_simple_dictionary_input(self): - input_data = { - 'Main theme': 'Test', - 'Type of PR': 'Test type', - 'Relevant tests added': 'no', - 'Focused PR': 'Yes', - 'General PR suggestions': 'general suggestion...', - 'Code feedback': [ - { - 'Code example': { - 'Before': 'Code before', - 'After': 'Code after' - } - }, - { - 'Code example': { - 'Before': 'Code before 2', - 'After': 'Code after 2' - } - } - ] - } - expected_output = """\ -- ๐ŸŽฏ **Main theme:** Test\n\ -- ๐Ÿ“Œ **Type of PR:** Test type\n\ -- ๐Ÿงช **Relevant tests added:** no\n\ -- โœจ **Focused PR:** Yes\n\ -- **General PR suggestions:** general suggestion...\n\n\n
๐Ÿค– Code feedback: - **Code example:**\n - **Before:**\n ```\n Code before\n ```\n - **After:**\n ```\n Code after\n ```\n\n - **Code example:**\n - **Before:**\n ```\n Code before 2\n ```\n - **After:**\n ```\n Code after 2\n ```\n\n
\ -""" + input_data = {'review': { + 'estimated_effort_to_review_[1-5]': '1, because the changes are minimal and straightforward, focusing on a single functionality addition.\n', + 'relevant_tests_added': 'No\n', 'possible_issues': 'No\n', 'security_concerns': 'No\n'}, 'code_feedback': [ + {'relevant_file': '``pr_agent/git_providers/git_provider.py\n``', 'language': 'python\n', + 'suggestion': "Consider raising an exception or logging a warning when 'pr_url' attribute is not found. This can help in debugging issues related to the absence of 'pr_url' in instances where it's expected. [important]\n", + 'relevant_line': '[return ""](https://github.com/Codium-ai/pr-agent-pro/pull/102/files#diff-52d45f12b836f77ed1aef86e972e65404634ea4e2a6083fb71a9b0f9bb9e062fR199)'}]} + + expected_output = '## PR Review\n\n
     PR feedback                    
\n\n\n\n\n\n
     PR feedback                    
โฑ๏ธ Estimated effort to review [1-5]\n\n1, because the changes are minimal and straightforward, focusing on a single functionality addition.\n\n\n
๐Ÿงช Relevant tests added\n\nNo\n\n\n
๐Ÿ” Possible issues\n\nNo\n\n\n
๐Ÿ”’ Security concerns\n\nNo\n\n\n
\n\n\n
Code feedback:\n\n
relevant filepr_agent/git_providers/git_provider.py\n
suggestion      \n\n\n\nConsider raising an exception or logging a warning when \'pr_url\' attribute is not found. This can help in debugging issues related to the absence of \'pr_url\' in instances where it\'s expected. [important]\n\n\n
relevant linereturn ""
\n\n
' + assert convert_to_markdown(input_data).strip() == expected_output.strip() # Tests that the function works correctly with an empty dictionary input def test_empty_dictionary_input(self): input_data = {} - expected_output = "" - assert convert_to_markdown(input_data).strip() == expected_output.strip() - def test_dictionary_input_containing_only_empty_dictionaries(self): - input_data = { - 'Main theme': {}, - 'Type of PR': {}, - 'Relevant tests added': {}, - 'Focused PR': {}, - 'General PR suggestions': {}, - 'Code suggestions': {} - } expected_output = '' + + assert convert_to_markdown(input_data).strip() == expected_output.strip() + def test_dictionary_with_empty_dictionaries(self): + input_data = {'review': {}, 'code_feedback': [{}]} + + expected_output = '' + + + assert convert_to_markdown(input_data).strip() == expected_output.strip() class TestBR: def test_br1(self): @@ -106,8 +85,9 @@ class TestBR: # print(file_change_description_br) def test_br2(self): - file_change_description = ('- Created a - new -class `ColorPaletteResourcesCollection ColorPaletteResourcesCollection ' - 'ColorPaletteResourcesCollection ColorPaletteResourcesCollection`') + file_change_description = ( + '- Created a - new -class `ColorPaletteResourcesCollection ColorPaletteResourcesCollection ' + 'ColorPaletteResourcesCollection ColorPaletteResourcesCollection`') file_change_description_br = insert_br_after_x_chars(file_change_description) expected_output = ('
  • Created a - new -class ColorPaletteResourcesCollection
    ' 'ColorPaletteResourcesCollection ColorPaletteResourcesCollection ' From 4175e8c4671727ad805f2087a97c4d56995b89c3 Mon Sep 17 00:00:00 2001 From: "Hussam.lawen" Date: Thu, 8 Feb 2024 20:49:42 +0200 Subject: [PATCH 07/12] fix test2 --- tests/unittest/test_load_yaml.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/unittest/test_load_yaml.py b/tests/unittest/test_load_yaml.py index 4cf436bf..bc91d743 100644 --- a/tests/unittest/test_load_yaml.py +++ b/tests/unittest/test_load_yaml.py @@ -38,14 +38,17 @@ PR Feedback: assert load_yaml(yaml_str) == expected_output def test_load_invalid_yaml2(self): - yaml_str = '''\ + yaml_str = '''\ - relevant file: src/app.py: suggestion content: The print statement is outside inside the if __name__ ==: \ ''' - with pytest.raises(ScannerError): - yaml.safe_load(yaml_str) + with pytest.raises(ScannerError): + yaml.safe_load(yaml_str) + + expected_output = [{'relevant file': 'src/app.py:', + 'suggestion content': 'The print statement is outside inside the if __name__ ==: '}] + assert load_yaml(yaml_str) == expected_output + + - expected_output =[{'relevant_file': 'src/app.py:', - 'suggestion content': 'The print statement is outside inside the if __name__ ==: '}] - assert load_yaml(yaml_str) == expected_output From 24c575737c133f7d926184f196986af4a42a7d38 Mon Sep 17 00:00:00 2001 From: "Hussam.lawen" Date: Thu, 8 Feb 2024 23:05:56 +0200 Subject: [PATCH 08/12] fix2 --- tests/unittest/test_load_yaml.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/unittest/test_load_yaml.py b/tests/unittest/test_load_yaml.py index bc91d743..55bc1205 100644 --- a/tests/unittest/test_load_yaml.py +++ b/tests/unittest/test_load_yaml.py @@ -34,14 +34,14 @@ PR Feedback: with pytest.raises(ScannerError): yaml.safe_load(yaml_str) - expected_output = {'PR Analysis': {'Main theme': 'Enhancing the `/describe` command prompt by adding title and description', 'Type of PR': 'Enhancement', 'Relevant tests added': False, 'Focused PR': 'Yes, the PR is focused on enhancing the `/describe` command prompt.'}, 'PR Feedback': {'General suggestions': 'The PR seems to be well-structured and focused on a specific enhancement. However, it would be beneficial to add tests to ensure the new feature works as expected.', 'Code feedback': [{'relevant_file': 'pr_agent/settings/pr_description_prompts.toml', 'suggestion': "Consider using a more descriptive variable name than 'user' for the command prompt. A more descriptive name would make the code more readable and maintainable. [medium]", 'relevant_line': 'user="""PR Info: aaa'}], 'Security concerns': False}} + expected_output = {'PR Analysis': {'Main theme': 'Enhancing the `/describe` command prompt by adding title and description', 'Type of PR': 'Enhancement', 'Relevant tests added': False, 'Focused PR': 'Yes, the PR is focused on enhancing the `/describe` command prompt.'}, 'PR Feedback': {'General suggestions': 'The PR seems to be well-structured and focused on a specific enhancement. However, it would be beneficial to add tests to ensure the new feature works as expected.', 'Code feedback': [{'relevant file': 'pr_agent/settings/pr_description_prompts.toml', 'suggestion': "Consider using a more descriptive variable name than 'user' for the command prompt. A more descriptive name would make the code more readable and maintainable. [medium]", 'relevant line': 'user="""PR Info: aaa'}], 'Security concerns': False}} assert load_yaml(yaml_str) == expected_output def test_load_invalid_yaml2(self): - yaml_str = '''\ -- relevant file: src/app.py: - suggestion content: The print statement is outside inside the if __name__ ==: \ - ''' + yaml_str = \ +'''\ + - relevant file: src/app.py: + suggestion content: The print statement is outside inside the if __name__ ==: ''' with pytest.raises(ScannerError): yaml.safe_load(yaml_str) From 5af9e8e74998746e83d305dd8ebc118116a5cbfc Mon Sep 17 00:00:00 2001 From: "Hussam.lawen" Date: Thu, 8 Feb 2024 23:53:29 +0200 Subject: [PATCH 09/12] fix --- pr_agent/algo/utils.py | 2 +- tests/unittest/test_load_yaml.py | 17 ++++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/pr_agent/algo/utils.py b/pr_agent/algo/utils.py index 7b189fca..d2a08a04 100644 --- a/pr_agent/algo/utils.py +++ b/pr_agent/algo/utils.py @@ -333,7 +333,7 @@ def load_yaml(response_text: str, keys_fix_yaml: List[str] = []) -> dict: def try_fix_yaml(response_text: str, keys_fix_yaml: List[str] = []) -> dict: response_text_lines = response_text.split('\n') - keys = ['relevant line:', 'suggestion content:', 'relevant_file:', 'existing code:', 'improved code:'] + keys = ['relevant line:', 'suggestion content:', 'relevant file:', 'existing code:', 'improved code:'] keys = keys + keys_fix_yaml # first fallback - try to convert 'relevant line: ...' to relevant line: |-\n ...' response_text_lines_copy = response_text_lines.copy() diff --git a/tests/unittest/test_load_yaml.py b/tests/unittest/test_load_yaml.py index 55bc1205..5736ae3b 100644 --- a/tests/unittest/test_load_yaml.py +++ b/tests/unittest/test_load_yaml.py @@ -38,16 +38,15 @@ PR Feedback: assert load_yaml(yaml_str) == expected_output def test_load_invalid_yaml2(self): - yaml_str = \ -'''\ - - relevant file: src/app.py: - suggestion content: The print statement is outside inside the if __name__ ==: ''' - with pytest.raises(ScannerError): - yaml.safe_load(yaml_str) + yaml_str = '''\ +- relevant file: src/app.py: + suggestion content: The print statement is outside inside the if __name__ ==: \ +''' + with pytest.raises(ScannerError): + yaml.safe_load(yaml_str) - expected_output = [{'relevant file': 'src/app.py:', - 'suggestion content': 'The print statement is outside inside the if __name__ ==: '}] - assert load_yaml(yaml_str) == expected_output + expected_output = [{'relevant file': 'src/app.py:', 'suggestion content': 'The print statement is outside inside the if __name__ ==: '}] + assert load_yaml(yaml_str) == expected_output From 555151602f8b51a45d0ac586bd108a20e95d67cc Mon Sep 17 00:00:00 2001 From: mrT23 Date: Fri, 9 Feb 2024 11:11:03 +0200 Subject: [PATCH 10/12] rstrip() --- pr_agent/git_providers/bitbucket_provider.py | 4 ++-- pr_agent/git_providers/bitbucket_server_provider.py | 4 ++-- pr_agent/git_providers/gitlab_provider.py | 4 ++-- pr_agent/tools/pr_reviewer.py | 4 +++- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/pr_agent/git_providers/bitbucket_provider.py b/pr_agent/git_providers/bitbucket_provider.py index 6d701ad4..a5b9d801 100644 --- a/pr_agent/git_providers/bitbucket_provider.py +++ b/pr_agent/git_providers/bitbucket_provider.py @@ -240,8 +240,8 @@ class BitbucketProvider(GitProvider): def generate_link_to_relevant_line_number(self, suggestion) -> str: try: - relevant_file = suggestion['relevant_file'].strip('`').strip("'") - relevant_line_str = suggestion['relevant_line'] + relevant_file = suggestion['relevant_file'].strip('`').strip("'").rstrip() + relevant_line_str = suggestion['relevant_line'].rstrip() if not relevant_line_str: return "" diff --git a/pr_agent/git_providers/bitbucket_server_provider.py b/pr_agent/git_providers/bitbucket_server_provider.py index df1492dd..c8ac30f2 100644 --- a/pr_agent/git_providers/bitbucket_server_provider.py +++ b/pr_agent/git_providers/bitbucket_server_provider.py @@ -246,8 +246,8 @@ class BitbucketServerProvider(GitProvider): def generate_link_to_relevant_line_number(self, suggestion) -> str: try: - relevant_file = suggestion['relevant_file'].strip('`').strip("'") - relevant_line_str = suggestion['relevant_line'] + relevant_file = suggestion['relevant_file'].strip('`').strip("'").rstrip() + relevant_line_str = suggestion['relevant_line'].rstrip() if not relevant_line_str: return "" diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index 889776af..f5d9f8e6 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -450,8 +450,8 @@ class GitLabProvider(GitProvider): def generate_link_to_relevant_line_number(self, suggestion) -> str: try: - relevant_file = suggestion['relevant_file'].strip('`').strip("'") - relevant_line_str = suggestion['relevant_line'] + relevant_file = suggestion['relevant_file'].strip('`').strip("'").rstrip() + relevant_line_str = suggestion['relevant_line'].rstrip() if not relevant_line_str: return "" diff --git a/pr_agent/tools/pr_reviewer.py b/pr_agent/tools/pr_reviewer.py index f6318a31..3f61b362 100644 --- a/pr_agent/tools/pr_reviewer.py +++ b/pr_agent/tools/pr_reviewer.py @@ -261,7 +261,9 @@ class PRReviewer: if get_settings().pr_reviewer.num_code_suggestions == 0: return - data = load_yaml(self.prediction.strip()) + data = load_yaml(self.prediction.strip(), + keys_fix_yaml=["estimated_effort_to_review_[1-5]:", "security_concerns:", "possible_issues:", + "relevant_file:", "relevant_line:", "suggestion:"]) comments: List[str] = [] for suggestion in data.get('PR Feedback', {}).get('Code feedback', []): relevant_file = suggestion.get('relevant_file', '').strip() From 6837e43114ae069aea66e54691df041dcf26cdc2 Mon Sep 17 00:00:00 2001 From: mrT23 Date: Fri, 9 Feb 2024 11:30:28 +0200 Subject: [PATCH 11/12] help --- pr_agent/algo/utils.py | 1 - pr_agent/servers/help.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/pr_agent/algo/utils.py b/pr_agent/algo/utils.py index d2a08a04..c2a8d066 100644 --- a/pr_agent/algo/utils.py +++ b/pr_agent/algo/utils.py @@ -43,7 +43,6 @@ def convert_to_markdown(output_data: dict, gfm_supported: bool=True) -> str: "Relevant tests added": "๐Ÿงช", "Focused PR": "โœจ", "Security concerns": "๐Ÿ”’", - "General suggestions": "๐Ÿ’ก", "Insights from user's answers": "๐Ÿ“", "Code feedback": "๐Ÿค–", "Estimated effort to review [1-5]": "โฑ๏ธ", diff --git a/pr_agent/servers/help.py b/pr_agent/servers/help.py index 36bd0db5..d93a6791 100644 --- a/pr_agent/servers/help.py +++ b/pr_agent/servers/help.py @@ -48,7 +48,7 @@ Examples for extra instructions: ``` [pr_reviewer] # /review # extra_instructions=""" -In the 'general suggestions' section, emphasize the following: +In the 'possible issues' section, emphasize the following: - Does the code logic cover relevant edge cases? - Is the code logic clear and easy to understand? - Is the code logic efficient? From 796e203c01497eb058f69dc28a39e2d1b4e4371d Mon Sep 17 00:00:00 2001 From: mrT23 Date: Fri, 9 Feb 2024 11:45:12 +0200 Subject: [PATCH 12/12] rstrip() --- pr_agent/tools/pr_description.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pr_agent/tools/pr_description.py b/pr_agent/tools/pr_description.py index 135b850b..0cdbbd85 100644 --- a/pr_agent/tools/pr_description.py +++ b/pr_agent/tools/pr_description.py @@ -377,7 +377,7 @@ class PRDescription: else: pr_body += f"""""" for filename, file_changes_title, file_change_description in list_tuples: - filename = filename.replace("'", "`") + filename = filename.replace("'", "`").rstrip() filename_publish = filename.split("/")[-1] file_changes_title_br = insert_br_after_x_chars(file_changes_title, x=(delta - 5)) file_changes_title_extended = file_changes_title_br.strip() + ""