Skip to content
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

Sweep: PR description using commit messages may produce wrong results #2

Open
mrT23 opened this issue Aug 7, 2023 · 1 comment · May be fixed by #4
Open

Sweep: PR description using commit messages may produce wrong results #2

mrT23 opened this issue Aug 7, 2023 · 1 comment · May be fixed by #4
Labels
sweep Assigns Sweep to an issue or pull request.

Comments

@mrT23
Copy link
Owner

mrT23 commented Aug 7, 2023

I've noticed that when calling the /describe command, the prompt uses the commit message in the PR to generate the PR description. In cases where a commit is no longer relevant (for example - it was later reverted in the same PR) this produces the wrong output in the PR description.

See this small PR example:

zmeir#2
In this example I've made 2 commits:

The first commit updates the docstring of some function
The second commit reverts the docstring to its original state and updates the max number of tokens in another file
After these 2 commits I called the /describe command, and as you can see, the PR title and description say that this PR includes changes to the docstring, but if you look at the diff with the main branch you can clearly see this is not true.

@sweep-ai sweep-ai bot added the sweep Assigns Sweep to an issue or pull request. label Aug 7, 2023
@sweep-ai
Copy link

sweep-ai bot commented Aug 7, 2023

Here's the PR! #4.

⚡ Sweep Free Trial: I used GPT-4 to create this ticket. You have 5 GPT-4 tickets left. For more GPT-4 tickets, visit our payment portal.To get Sweep to recreate this ticket, leave a comment prefixed with "sweep:" or edit the issue.


Step 1: 🔍 Code Search

I found the following snippets in your repository. I will now analyze these snippets and come up with a plan.

Some code snippets I looked at (click to expand). If some file is missing from here, you can mention the path in the ticket description.

def publish_inline_comments(self, comments: list[dict]):
raise NotImplementedError('Publishing inline comments is not implemented for the local git provider')
def publish_code_suggestion(self, body: str, relevant_file: str,
relevant_lines_start: int, relevant_lines_end: int):
raise NotImplementedError('Publishing code suggestions is not implemented for the local git provider')
def publish_code_suggestions(self, code_suggestions: list):
raise NotImplementedError('Publishing code suggestions is not implemented for the local git provider')
def publish_labels(self, labels):
pass # Not applicable to the local git provider, but required by the interface
def remove_initial_comment(self):
pass # Not applicable to the local git provider, but required by the interface
def get_languages(self):
"""
Calculate percentage of languages in repository. Used for hunk prioritisation.
"""
# Get all files in repository
filepaths = [Path(item.path) for item in self.repo.tree().traverse() if item.type == 'blob']
# Identify language by file extension and count
lang_count = Counter(ext.lstrip('.') for filepath in filepaths for ext in [filepath.suffix.lower()])
# Convert counts to percentages
total_files = len(filepaths)
lang_percentage = {lang: count / total_files * 100 for lang, count in lang_count.items()}
return lang_percentage
def get_pr_branch(self):
return self.repo.head
def get_user_id(self):
return -1 # Not used anywhere for the local provider, but required by the interface
def get_pr_description(self):
commits_diff = list(self.repo.iter_commits(self.target_branch_name + '..HEAD'))
# Get the commit messages and concatenate
commit_messages = " ".join([commit.message for commit in commits_diff])
# TODO Handle the description better - maybe use gpt-3.5 summarisation here?
return commit_messages[:200] # Use max 200 characters
def get_pr_title(self):
"""
Substitutes the branch-name as the PR-mimic title.
"""
return self.head_branch_name
def get_issue_comments(self):
raise NotImplementedError('Getting issue comments is not implemented for the local git provider')
def get_labels(self):
raise NotImplementedError('Getting labels is not implemented for the local git provider')

import copy
import json
import logging
from typing import List, Tuple
from jinja2 import Environment, StrictUndefined
from pr_agent.algo.ai_handler import AiHandler
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.config_loader import get_settings
from pr_agent.git_providers import get_git_provider
from pr_agent.git_providers.git_provider import get_main_pr_language
class PRDescription:
def __init__(self, pr_url: str, args: list = None):
"""
Initialize the PRDescription object with the necessary attributes and objects for generating a PR description
using an AI model.
Args:
pr_url (str): The URL of the pull request.
args (list, optional): List of arguments passed to the PRDescription class. Defaults to None.
"""
# Initialize the git provider and main PR language
self.git_provider = get_git_provider()(pr_url)
self.main_pr_language = get_main_pr_language(
self.git_provider.get_languages(), self.git_provider.get_files()
)
# Initialize the AI handler
self.ai_handler = AiHandler()
# Initialize the variables dictionary
self.vars = {
"title": self.git_provider.pr.title,
"branch": self.git_provider.get_pr_branch(),
"description": self.git_provider.get_pr_description(),
"language": self.main_pr_language,
"diff": "", # empty diff for initial calculation
"extra_instructions": get_settings().pr_description.extra_instructions,
"commit_messages_str": self.git_provider.get_commit_messages()
}
# Initialize the token handler
self.token_handler = TokenHandler(
self.git_provider.pr,
self.vars,
get_settings().pr_description_prompt.system,
get_settings().pr_description_prompt.user,
)
# Initialize patches_diff and prediction attributes
self.patches_diff = None
self.prediction = None
async def run(self):
"""
Generates a PR description using an AI model and publishes it to the PR.
"""
logging.info('Generating a PR description...')
if get_settings().config.publish_output:
self.git_provider.publish_comment("Preparing pr description...", is_temporary=True)
await retry_with_fallback_models(self._prepare_prediction)
logging.info('Preparing answer...')
pr_title, pr_body, pr_types, markdown_text = self._prepare_pr_answer()
if get_settings().config.publish_output:
logging.info('Pushing answer...')
if get_settings().pr_description.publish_description_as_comment:
self.git_provider.publish_comment(markdown_text)
else:
self.git_provider.publish_description(pr_title, pr_body)
if self.git_provider.is_supported("get_labels"):
current_labels = self.git_provider.get_labels()
if current_labels is None:
current_labels = []
self.git_provider.publish_labels(pr_types + current_labels)
self.git_provider.remove_initial_comment()
return ""
async def _prepare_prediction(self, model: str) -> None:
"""
Prepare the AI prediction for the PR description based on the provided model.
Args:
model (str): The name of the model to be used for generating the prediction.
Returns:
None
Raises:
Any exceptions raised by the 'get_pr_diff' and '_get_prediction' functions.
"""
logging.info('Getting PR diff...')
self.patches_diff = get_pr_diff(self.git_provider, self.token_handler, model)
logging.info('Getting AI prediction...')
self.prediction = await self._get_prediction(model)
async def _get_prediction(self, model: str) -> str:
"""
Generate an AI prediction for the PR description based on the provided model.
Args:
model (str): The name of the model to be used for generating the prediction.
Returns:
str: The generated AI prediction.
"""
variables = copy.deepcopy(self.vars)
variables["diff"] = self.patches_diff # update diff
environment = Environment(undefined=StrictUndefined)
system_prompt = environment.from_string(get_settings().pr_description_prompt.system).render(variables)
user_prompt = environment.from_string(get_settings().pr_description_prompt.user).render(variables)

[pr_description_prompt]
system="""You are CodiumAI-PR-Reviewer, a language model designed to review git pull requests.
Your task is to provide full description of the PR content.
- Make sure not to focus the new PR code (the '+' lines).
{%- if extra_instructions %}
Extra instructions from the user:
{{ extra_instructions }}
{% endif %}
You must use the following JSON schema to format your answer:
```json
{
"PR Title": {
"type": "string",
"description": "an informative title for the PR, describing its main theme"
},
"PR Type": {
"type": "string",
"description": possible values are: ["Bug fix", "Tests", "Bug fix with tests", "Refactoring", "Enhancement", "Documentation", "Other"]
},
"PR Description": {
"type": "string",
"description": "an informative and concise description of the PR"
},
"PR Main Files Walkthrough": {
"type": "string",
"description": "a walkthrough of the PR changes. Review main files, in bullet points, and shortly describe the changes in each file (up to 10 most important files). Format: -`filename`: description of changes\n..."
}
}
Don't repeat the prompt in the answer, and avoid outputting the 'type' and 'description' fields.
"""
user="""PR Info:
Branch: '{{branch}}'
{%- if language %}
Main language: {{language}}
{%- endif %}
{%- if commit_messages_str %}
Commit messages:
{{commit_messages_str}}
{%- endif %}
The PR Git Diff:
```
{{diff}}
```
Note that lines in the diff body are prefixed with a symbol that represents the type of change: '-' for deletions, '+' for additions, and ' ' (a space) for unchanged lines.
Response (should be a valid JSON, and nothing else):
```json
"""

async with session.get(NOTIFICATION_URL, headers=headers, params=params) as response:
if response.status == 200:
if 'Last-Modified' in response.headers:
last_modified[0] = response.headers['Last-Modified']
since[0] = None
notifications = await response.json()
if not notifications:
continue
for notification in notifications:
handled_ids.add(notification['id'])
if 'reason' in notification and notification['reason'] == 'mention':
if 'subject' in notification and notification['subject']['type'] == 'PullRequest':
pr_url = notification['subject']['url']
latest_comment = notification['subject']['latest_comment_url']
async with session.get(latest_comment, headers=headers) as comment_response:
if comment_response.status == 200:
comment = await comment_response.json()
if 'id' in comment:
if comment['id'] in handled_ids:
continue
else:
handled_ids.add(comment['id'])
if 'user' in comment and 'login' in comment['user']:
if comment['user']['login'] == user_id:
continue
comment_body = comment['body'] if 'body' in comment else ''
commenter_github_user = comment['user']['login'] \
if 'user' in comment else ''
logging.info(f"Commenter: {commenter_github_user}\nComment: {comment_body}")
user_tag = "@" + user_id
if user_tag not in comment_body:
continue
rest_of_comment = comment_body.split(user_tag)[1].strip()
success = await agent.handle_request(pr_url, rest_of_comment)
if not success:
git_provider.set_pr(pr_url)
git_provider.publish_comment("### How to use PR-Agent\n" +
bot_help_text(user_id))
elif response.status != 304:
print(f"Failed to fetch notifications. Status code: {response.status}")
except Exception as e:
logging.error(f"Exception during processing of a notification: {e}")
if __name__ == '__main__':

[pr_review_prompt]
system="""You are CodiumAI-PR-Reviewer, a language model designed to review git pull requests.
Your task is to provide constructive and concise feedback for the PR, and also provide meaningfull code suggestions to improve the new PR code (the '+' lines).
{%- if num_code_suggestions > 0 %}
- Provide up to {{ num_code_suggestions }} code suggestions.
- Try to focus on the most important suggestions, like fixing code problems, issues and bugs. As a second priority, provide suggestions for meaningfull code improvements, like performance, vulnerability, modularity, and best practices.
- Suggestions should focus on improving the new added code lines.
- Make sure not to provide suggestions repeating modifications already implemented in the new PR code (the '+' lines).
{%- endif %}
{%- if extra_instructions %}
Extra instructions from the user:
{{ extra_instructions }}
{% endif %}
You must use the following JSON schema to format your answer:
```json
{
"PR Analysis": {
"Main theme": {
"type": "string",
"description": "a short explanation of the PR"
},
"Type of PR": {
"type": "string",
"enum": ["Bug fix", "Tests", "Refactoring", "Enhancement", "Documentation", "Other"]
},
{%- 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."
},
{%- endif %}
{%- if require_tests %}
"Relevant tests added": {
"type": "string",
"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"
},
{%- 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."
}
},
{%- endif %}
"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."
},
{%- 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"
},
"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 line should be a '+' line. Make sure to output the line exactly as it appears in the relevant file"
}
}
},
{%- endif %}
{%- if require_security %}
"Security concerns": {
"type": "string",
"description": "yes\\no question: does this PR code introduce possible security concerns or issues, like SQL injection, XSS, CSRF, and others ? If answered 'yes', explain your answer shortly"
? explain your answer shortly"
}
{%- endif %}
}
}
```
Example output:
'
{
"PR Analysis":
{
"Main theme": "xxx",
"Type of PR": "Bug fix",
{%- if require_score %}
"Score": 89,
{%- endif %}
{%- if require_tests %}
"Relevant tests added": "No",
{%- endif %}
{%- if require_focused %}
"Focused PR": "yes\\no, because ..."
{%- endif %}
},
"PR Feedback":
{
"General PR suggestions": "..., `xxx`...",
{%- if num_code_suggestions > 0 %}
"Code feedback": [
{
"relevant file": "directory/xxx.py",
"suggestion": "xxx [important]",
"relevant line": "xxx",
},
...
]
{%- endif %}
{%- if require_security %}
"Security concerns": "No, because ..."
{%- endif %}
}
}
'
Don't repeat the prompt in the answer, and avoid outputting the 'type' and 'description' fields.
"""
user="""PR Info:
Title: '{{title}}'
Branch: '{{branch}}'
Description: '{{description}}'
{%- if language %}
Main language: {{language}}
{%- endif %}
{%- if commit_messages_str %}
Commit messages:
{{commit_messages_str}}
{%- endif %}
{%- if question_str %}
######
Here are questions to better understand the PR. Use the answers to provide better feedback.
{{question_str|trim}}
User answers:
{{answer_str|trim}}
######
{%- endif %}
The PR Git Diff:
```
{{diff}}
```
Note that lines in the diff body are prefixed with a symbol that represents the type of change: '-' for deletions, '+' for additions, and ' ' (a space) for unchanged lines.
Response (should be a valid JSON, and nothing else):
```json
"""

I also found the following external resources that might be helpful:

Summaries of links found in the content:

zmeir#2:

The page is a pull request on GitHub titled "Update max tokens and docstring" by the user "zmeir". The pull request includes two commits. The first commit updates the docstring of a function, while the second commit reverts the docstring back to its original state and updates the maximum number of tokens in another file. The user is experiencing an issue where the /describe command in GitHub uses the commit message to generate the pull request description, which is causing incorrect information to be displayed in the description. The user provides an example of this issue and explains that the PR title and description incorrectly state that the PR includes changes to the docstring, even though the diff with the main branch shows that this is not true.


Step 2: 🧐 Snippet Analysis

From looking through the relevant snippets, I decided to make the following modifications:

File Path Proposed Changes
pr_agent/git_providers/local_git_provider.py Modify the get_pr_description method to exclude the messages of reverted commits when generating the PR description. Use the new method from the pr_processing.py file to identify reverted commits.
pr_agent/algo/pr_processing.py Modify the get_pr_diff method to return a structured representation of the diff. Add a new method to compare the changes introduced by each commit with the final PR diff and identify reverted commits.

Step 3: 📝 Planning

I have created a plan for writing the pull request. I am now working my plan and coding the required changes to address this issue. Here is the planned pull request:

Fix PR description generation when commits are reverted
sweep/fix-pr-description-generation

Description

This PR fixes the issue with PR description generation when commits are reverted. Currently, the PR description is generated by concatenating all commit messages, which can lead to incorrect descriptions if some commits have been reverted. This PR introduces a check for reverted commits and excludes their messages from the PR description.

Summary of Changes

  • Modified the get_pr_description method in local_git_provider.py to exclude the messages of reverted commits when generating the PR description.
  • Modified the get_pr_diff method in pr_processing.py to return a structured representation of the diff.
  • Added a new method in pr_processing.py to compare the changes introduced by each commit with the final PR diff and identify reverted commits.

Please review and merge this PR to ensure accurate PR descriptions when commits are reverted.


Step 4: ⌨️ Coding

File Instructions Progress
pr_agent/git_providers/local_git_provider.py Modify the get_pr_description method to exclude the messages of reverted commits when generating the PR description. Use the new method from the pr_processing.py file to identify reverted commits. ✅ Done with commit 317ac9f
pr_agent/algo/pr_processing.py Modify the get_pr_diff method to return a structured representation of the diff. Add a new method to compare the changes introduced by each commit with the final PR diff and identify reverted commits. ✅ Done with commit 317ac9f

Step 5: 🔁 Code Review

Here are the my self-reviews of my changes at sweep/fix-pr-description-generation.

Here is the 1st review

Thanks for your contribution. Here are a few things that need to be addressed:

  • In pr_agent/algo/pr_processing.py, the return type of the function get_pr_diff has been changed from str to Tuple[List[str], int]. This might cause issues in other parts of the code where this function is called and the returned value is expected to be a string. Please ensure that all calls to this function are updated to handle the new return type.

  • Also in pr_agent/algo/pr_processing.py, the new function identify_reverted_commits uses attributes and methods (diff, a_path, filename, patch) that might not exist in the Commit and FilePatchInfo classes. Please ensure that these classes have the required attributes and methods.

  • In pr_agent/git_providers/local_git_provider.py, the method get_pr_description now calls the get_diff_files method. Please ensure that this method exists and returns the correct type of value.

Please address these issues and update the pull request. If you need any help, feel free to ask.

I finished incorporating these changes.


To recreate the pull request, leave a comment prefixed with "sweep:" or edit the issue.
Join Our Discord

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sweep Assigns Sweep to an issue or pull request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant