Skip to content

Commit

Permalink
Update PR review prompts and terminology for clarity and consistency (#…
Browse files Browse the repository at this point in the history
…954)

* Update PR review prompts and terminology for clarity and consistency
  • Loading branch information
mrT23 authored Jun 10, 2024
1 parent f1d5587 commit aac7aea
Show file tree
Hide file tree
Showing 9 changed files with 57 additions and 36 deletions.
51 changes: 33 additions & 18 deletions docs/docs/tools/review.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
## Overview
The `review` tool scans the PR code changes, and automatically generates a PR review.
The `review` tool scans the PR code changes, and generates a list of feedbacks about the PR, aiming to aid the reviewing process.
<br>
The tool can be triggered automatically every time a new PR is [opened](../usage-guide/automations_and_usage.md#github-app-automatic-tools-when-a-new-pr-is-opened), or can be invoked manually by commenting on any PR:
```
/review
```

Note that the main purpose of the `review` tool is to provide the **PR reviewer** with useful feedbacks and insights. The PR author, in contrast, may prefer to save time and focus on the output of the [improve](./improve.md) tool, which provides actionable code suggestions.

## Example usage

### Manual triggering
Expand Down Expand Up @@ -50,19 +53,27 @@ Note that the incremental mode is only available for GitHub.

![incremental review](https://codium.ai/images/pr_agent/incremental_review_2.png){width=512}

### PR Reflection
[//]: # (### PR Reflection)

By invoking:
```
/reflect_and_review
```
The tool will first ask the author questions about the PR, and will guide the review based on their answers.
[//]: # ()
[//]: # (By invoking:)

[//]: # (```)

![reflection questions](https://codium.ai/images/pr_agent/reflection_questions.png){width=512}
[//]: # (/reflect_and_review)

![reflection answers](https://codium.ai/images/pr_agent/reflection_answers.png){width=512}
[//]: # (```)

![reflection insights](https://codium.ai/images/pr_agent/reflection_insights.png){width=512}
[//]: # (The tool will first ask the author questions about the PR, and will guide the review based on their answers.)

[//]: # ()
[//]: # (![reflection questions]&#40;https://codium.ai/images/pr_agent/reflection_questions.png&#41;{width=512})

[//]: # ()
[//]: # (![reflection answers]&#40;https://codium.ai/images/pr_agent/reflection_answers.png&#41;{width=512})

[//]: # ()
[//]: # (![reflection insights]&#40;https://codium.ai/images/pr_agent/reflection_insights.png&#41;{width=512})



Expand Down Expand Up @@ -167,7 +178,7 @@ If enabled, the `review` tool can approve a PR when a specific comment, `/review

!!! tip "General guidelines"

The `review` tool provides a collection of possible feedbacks about a PR.
The `review` tool provides a collection of configurable feedbacks about a PR.
It is recommended to review the [Configuration options](#configuration-options) section, and choose the relevant options for your use case.

Some of the features that are disabled by default are quite useful, and should be considered for enabling. For example:
Expand All @@ -183,13 +194,6 @@ If enabled, the `review` tool can approve a PR when a specific comment, `/review
Meaning the `review` tool will run automatically on every PR, without providing code suggestions.
Edit this field to enable/disable the tool, or to change the used configurations.

!!! tip "Code suggestions"

If you set `num_code_suggestions`>0 , the `review` tool will also provide code suggestions.

Notice If you are interested **only** in the code suggestions, it is recommended to use the [`improve`](./improve.md) feature instead, since it is a dedicated only to code suggestions, and usually gives better results.
Use the `review` tool if you want to get more comprehensive feedback, which includes code suggestions as well.

!!! tip "Possible labels from the review tool"

The `review` tool can auto-generate two specific types of labels for a PR:
Expand Down Expand Up @@ -244,3 +248,14 @@ If enabled, the `review` tool can approve a PR when a specific comment, `/review
[pr_reviewer]
maximal_review_effort = 5
```

[//]: # (!!! tip "Code suggestions")

[//]: # ()
[//]: # ( If you set `num_code_suggestions`>0 , the `review` tool will also provide code suggestions.)

[//]: # ( )
[//]: # ( Notice If you are interested **only** in the code suggestions, it is recommended to use the [`improve`]&#40;./improve.md&#41; feature instead, since it is a dedicated only to code suggestions, and usually gives better results.)

[//]: # ( Use the `review` tool if you want to get more comprehensive feedback, which includes code suggestions as well.)

9 changes: 5 additions & 4 deletions pr_agent/algo/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def emphasize_header(text: str) -> str:
# Splitting the string and wrapping the first part in <strong> tags
if colon_position != -1:
# Everything before the colon (inclusive) is wrapped in <strong> tags
transformed_string = "<strong>" + text[:colon_position + 1] + "</strong>" + text[colon_position + 1:]
transformed_string = "<strong>" + text[:colon_position + 1] + "</strong>" +'<br>' + text[colon_position + 1:]
else:
# If there's no ": ", return the original string
transformed_string = text
Expand Down Expand Up @@ -74,6 +74,7 @@ def convert_to_markdown(output_data: dict, gfm_supported: bool = True, increment
emojis = {
"Can be split": "🔀",
"Possible issues": "⚡",
"Key issues to review": "⚡",
"Score": "🏅",
"Relevant tests": "🧪",
"Focused PR": "✨",
Expand All @@ -85,9 +86,9 @@ def convert_to_markdown(output_data: dict, gfm_supported: bool = True, increment
}
markdown_text = ""
if not incremental_review:
markdown_text += f"## PR Review 🔍\n\n"
markdown_text += f"## PR Reviewer Guide 🔍\n\n"
else:
markdown_text += f"## Incremental PR Review 🔍\n\n"
markdown_text += f"## Incremental PR Reviewer Guide 🔍\n\n"
markdown_text += f"⏮️ Review for commits since previous PR-Agent review {incremental_review}.\n\n"
if gfm_supported:
markdown_text += "<table>\n<tr>\n"
Expand All @@ -110,7 +111,7 @@ def convert_to_markdown(output_data: dict, gfm_supported: bool = True, increment
markdown_text += f"<tr><td> {emoji}&nbsp;<strong>{key_nice}</strong></td><td>\n\n{value}\n\n</td></tr>\n"
elif 'can be split' in key_nice.lower():
markdown_text += process_can_be_split(emoji, value)
elif 'possible issues' in key_nice.lower():
elif 'key issues to review' in key_nice.lower():
value = value.strip()
issues = value.split('\n- ')
for i, _ in enumerate(issues):
Expand Down
2 changes: 1 addition & 1 deletion pr_agent/git_providers/bitbucket_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ def publish_persistent_comment(self, pr_comment: str,
latest_commit_url = self.get_latest_commit_url()
comment_url = self.get_comment_url(comment)
if update_header:
updated_header = f"{initial_header}\n\n### ({name.capitalize()} updated until commit {latest_commit_url})\n"
updated_header = f"{initial_header}\n\n#### ({name.capitalize()} updated until commit {latest_commit_url})\n"
pr_comment_updated = pr_comment.replace(initial_header, updated_header)
else:
pr_comment_updated = pr_comment
Expand Down
6 changes: 3 additions & 3 deletions pr_agent/git_providers/github_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,9 @@ def get_previous_review(self, *, full: bool, incremental: bool):
self.comments = list(self.pr.get_issue_comments())
prefixes = []
if full:
prefixes.append("## PR Review")
prefixes.append("## PR Reviewer Guide")
if incremental:
prefixes.append("## Incremental PR Review")
prefixes.append("## Incremental PR Reviewer Guide")
for index in range(len(self.comments) - 1, -1, -1):
if any(self.comments[index].body.startswith(prefix) for prefix in prefixes):
return self.comments[index]
Expand Down Expand Up @@ -217,7 +217,7 @@ def publish_persistent_comment(self, pr_comment: str,
latest_commit_url = self.get_latest_commit_url()
comment_url = self.get_comment_url(comment)
if update_header:
updated_header = f"{initial_header}\n\n### ({name.capitalize()} updated until commit {latest_commit_url})\n"
updated_header = f"{initial_header}\n\n#### ({name.capitalize()} updated until commit {latest_commit_url})\n"
pr_comment_updated = pr_comment.replace(initial_header, updated_header)
else:
pr_comment_updated = pr_comment
Expand Down
2 changes: 1 addition & 1 deletion pr_agent/git_providers/gitlab_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ def publish_persistent_comment(self, pr_comment: str,
latest_commit_url = self.get_latest_commit_url()
comment_url = self.get_comment_url(comment)
if update_header:
updated_header = f"{initial_header}\n\n### ({name.capitalize()} updated until commit {latest_commit_url})\n"
updated_header = f"{initial_header}\n\n#### ({name.capitalize()} updated until commit {latest_commit_url})\n"
pr_comment_updated = pr_comment.replace(initial_header, updated_header)
else:
pr_comment_updated = pr_comment
Expand Down
2 changes: 1 addition & 1 deletion pr_agent/settings/configuration.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ require_security_review=true
require_soc2_ticket=false
soc2_ticket_prompt="Does the PR description include a link to ticket in a project management system (e.g., Jira, Asana, Trello, etc.) ?"
# general options
num_code_suggestions=4
num_code_suggestions=0
inline_code_comments = false
ask_and_reflect=false
#automatic_review=true
Expand Down
8 changes: 4 additions & 4 deletions pr_agent/settings/pr_reviewer_prompts.toml
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class SubPR(BaseModel):
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.")
estimated_effort_to_review_[1-5]: int = 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.")
{%- endif %}
{%- if require_score %}
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.")
Expand All @@ -68,7 +68,7 @@ class Review(BaseModel):
{%- if question_str %}
insights_from_user_answers: str = Field(description="shortly summarize the insights you gained from the user's answers to the questions")
{%- endif %}
possible_issues: str = Field(description="Does this PR code introduce clear issues, bugs, or major performance concerns? If there are no apparent issues, respond with 'No'. If there are any issues, describe them briefly. Use bullet points if more than one issue. Be specific, and provide examples if possible. Start each bullet point with a short specific header, such as: "- Possible Bug: ...", etc.")
key_issues_to_review: str = Field(description="Does this PR code introduce issues, bugs, or major performance concerns, which the PR reviewer should further investigate ? If there are no apparent issues, respond with 'None'. If there are any issues, describe them briefly. Use bullet points if more than one issue. Be specific, and provide examples if possible. Start each bullet point with a short specific header, such as: "- Possible Bug: ...", etc.")
{%- if require_security_review %}
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. If there are security concerns or issues, start your answer with a short header, such as: 'Sensitive information exposure: ...', 'SQL injection: ...' etc. Explain your answer. Be specific and give examples if possible")
{%- endif %}
Expand Down Expand Up @@ -101,14 +101,14 @@ Example output:
review:
{%- if require_estimate_effort_to_review %}
estimated_effort_to_review_[1-5]: |
3, because ...
3
{%- endif %}
{%- if require_score %}
score: 89
{%- endif %}
relevant_tests: |
No
possible_issues: |
key_issues_to_review: |
No
security_concerns: |
No
Expand Down
11 changes: 8 additions & 3 deletions pr_agent/tools/pr_reviewer.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ async def run(self) -> None:
if get_settings().pr_reviewer.persistent_comment and not self.incremental.is_incremental:
final_update_message = get_settings().pr_reviewer.final_update_message
self.git_provider.publish_persistent_comment(pr_review,
initial_header="## PR Review 🔍",
initial_header="## PR Reviewer Guide 🔍",
update_header=True,
final_update_message=final_update_message, )
else:
Expand Down Expand Up @@ -193,10 +193,15 @@ def _prepare_pr_review(self) -> str:
the feedback.
"""
data = load_yaml(self.prediction.strip(),
keys_fix_yaml=["estimated_effort_to_review_[1-5]:", "security_concerns:", "possible_issues:",
keys_fix_yaml=["estimated_effort_to_review_[1-5]:", "security_concerns:", "key_issues_to_review:",
"relevant_file:", "relevant_line:", "suggestion:"])
github_action_output(data, 'review')

# move data['review'] 'key_issues_to_review' key to the end of the dictionary
if 'key_issues_to_review' in data['review']:
key_issues_to_review = data['review'].pop('key_issues_to_review')
data['review']['key_issues_to_review'] = key_issues_to_review

if 'code_feedback' in data:
code_feedback = data['code_feedback']

Expand Down Expand Up @@ -260,7 +265,7 @@ def _publish_inline_code_comments(self) -> None:
return

data = load_yaml(self.prediction.strip(),
keys_fix_yaml=["estimated_effort_to_review_[1-5]:", "security_concerns:", "possible_issues:",
keys_fix_yaml=["estimated_effort_to_review_[1-5]:", "security_concerns:", "key_issues_to_review:",
"relevant_file:", "relevant_line:", "suggestion:"])
comments: List[str] = []
for suggestion in data.get('code_feedback', []):
Expand Down
2 changes: 1 addition & 1 deletion tests/unittest/test_convert_to_markdown.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def test_simple_dictionary_input(self):
'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<table>\n<tr>\n<tr><td> ⏱️&nbsp;<strong>Estimated&nbsp;effort&nbsp;to&nbsp;review [1-5]</strong></td><td>\n\n1, because the changes are minimal and straightforward, focusing on a single functionality addition.\n\n\n</td></tr>\n<tr><td> 🧪&nbsp;<strong>Relevant tests</strong></td><td>\n\nNo\n\n\n</td></tr>\n<tr><td> ⚡&nbsp;<strong>Possible issues</strong></td><td>\n\nNo\n\n</td></tr>\n<tr><td> 🔒&nbsp;<strong>Security concerns</strong></td><td>\n\nNo\n\n</td></tr>\n</table>\n\n\n<details><summary> <strong>Code feedback:</strong></summary>\n\n<hr><table><tr><td>relevant file</td><td>pr_agent/git_providers/git_provider.py\n</td></tr><tr><td>suggestion &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;</td><td>\n\n<strong>\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</strong>\n</td></tr><tr><td>relevant line</td><td><a href=\'https://github.com/Codium-ai/pr-agent-pro/pull/102/files#diff-52d45f12b836f77ed1aef86e972e65404634ea4e2a6083fb71a9b0f9bb9e062fR199\'>return ""</a></td></tr></table><hr>\n\n</details>'
expected_output = '## PR Reviewer Guide 🔍\n\n<table>\n<tr>\n<tr><td> ⏱️&nbsp;<strong>Estimated&nbsp;effort&nbsp;to&nbsp;review [1-5]</strong></td><td>\n\n1, because the changes are minimal and straightforward, focusing on a single functionality addition.\n\n\n</td></tr>\n<tr><td> 🧪&nbsp;<strong>Relevant tests</strong></td><td>\n\nNo\n\n\n</td></tr>\n<tr><td> ⚡&nbsp;<strong>Possible issues</strong></td><td>\n\nNo\n\n\n</td></tr>\n<tr><td> 🔒&nbsp;<strong>Security concerns</strong></td><td>\n\nNo\n\n</td></tr>\n</table>\n\n\n<details><summary> <strong>Code feedback:</strong></summary>\n\n<hr><table><tr><td>relevant file</td><td>pr_agent/git_providers/git_provider.py\n</td></tr><tr><td>suggestion &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;</td><td>\n\n<strong>\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</strong>\n</td></tr><tr><td>relevant line</td><td><a href=\'https://github.com/Codium-ai/pr-agent-pro/pull/102/files#diff-52d45f12b836f77ed1aef86e972e65404634ea4e2a6083fb71a9b0f9bb9e062fR199\'>return ""</a></td></tr></table><hr>\n\n</details>'

assert convert_to_markdown(input_data).strip() == expected_output.strip()

Expand Down

0 comments on commit aac7aea

Please sign in to comment.