Merge remote-tracking branch 'origin/main' into patch-1

CHANGELOG.md

@@ -1,3 +1,10 @@
+## 2023-08-03
+
+### Optimized
+- Optimized PR diff processing by introducing caching for diff files, reducing the number of API calls.
+- Refactored `load_large_diff` function to generate a patch only when necessary.
+- Fixed a bug in the GitLab provider where the new file was not retrieved correctly.
+
 ## 2023-08-02
 
 ### Enhanced

CONFIGURATION.md

@@ -1,9 +1,31 @@
 ## Configuration
 
-The different tools and sub-tools used by CodiumAI pr-agent are adjustable via the configuration file: `/pr-agent/settings/configuration.toml`.
+The different tools and sub-tools used by CodiumAI PR-Agent are adjustable via the configuration file: `/pr-agent/settings/configuration.toml`.
+
+### CLI
+When running from source (CLI), your local configuration file will be used.
+
+Example for invoking the 'review' tools via the CLI: 
+
+```
+python cli.py --pr-url=<pr_url>  review
+```
+In addition to general configurations, The 'review' tool will use parameters from the `[pr_reviewer]` section. Every tool has a dedicated section in the configuration file
+
+Note that you can print results locally, without publishing them, by setting in `configuration.toml`:
+
+```
+[config]
+publish_output=true
+verbosity_level=2
+```
+This is useful for debugging or experimenting with the different tools.
+
+### Working from pre-built repo (GitHub Action/GitHub App/Docker/...)
+When running PR-Agent from a pre-built repo, the default configuration file will be loaded.
 
 To edit the configuration of any tool, just add `--config_path=<value>` to you command.
-For example if you want to edit online the `pr_reviewer` configurations, you can run:
+For example if you want to edit online `pr_reviewer` configurations, you can run:
 ```
 /review --pr_reviewer.extra_instructions="focus on the file xyz" --pr_reviewer.require_score_review=false ...
 ```

README.md

@@ -65,7 +65,6 @@ CodiumAI `PR-Agent` is an open-source tool aiming to help developers review pull
 - [Overview](#overview)
 - [Try it now](#try-it-now)
 - [Installation](#installation)
-- [Usage and tools](#usage-and-tools)
 - [Configuration](./CONFIGURATION.md)
 - [How it works](#how-it-works)
 - [Why use PR-Agent](#why-use-pr-agent)
@@ -135,19 +134,11 @@ There are several ways to use PR-Agent:
 - [Method 5: Run as a GitHub App](INSTALL.md#method-5-run-as-a-github-app)
   - Allowing you to automate the review process on your private or public repositories
 
-## Usage and Tools
-
-**PR-Agent** provides six types of interactions ("tools"): `"PR Reviewer"`, `"PR Q&A"`, `"PR Description"`, `"PR Code Sueggestions"`, `"PR Reflect and Review"` and `"PR Update Changlog"`.
-
-- The "PR Reviewer" tool automatically analyzes PRs, and provides various types of feedback.
-- The "PR Q&A" tool answers free-text questions about the PR.
-- The "PR Description" tool automatically sets the PR Title and body.
-- The "PR Code Suggestion" tool provide inline code suggestions for the PR that can be applied and committed.
-- The "PR Reflect and Review" tool initiates a dialog with the user, asks them to reflect on the PR, and then provides a more focused review.
-- The "PR Update Changelog" tool automatically updates the CHANGELOG.md file with the PR changes.
 
 ## How it works
 
+The following diagram illustrates PR-Agent tools and their flow:
+
 ![PR-Agent Tools](https://www.codium.ai/wp-content/uploads/2023/07/codiumai-diagram-v4.jpg)
 
 Check out the [PR Compression strategy](./PR_COMPRESSION.md) page for more details on how we convert a code diff to a manageable LLM prompt
@@ -156,11 +147,11 @@ Check out the [PR Compression strategy](./PR_COMPRESSION.md) page for more detai
 
 A reasonable question that can be asked is: `"Why use PR-Agent? What make it stand out from existing tools?"`
 
-Here are some of the reasons why:
+Here are some advantages of PR-Agent:
 
 - We emphasize **real-life practical usage**. Each tool (review, improve, ask, ...) has a single GPT-4 call, no more. We feel that this is critical for realistic team usage - obtaining an answer quickly (~30 seconds) and affordably.
 - Our [PR Compression strategy](./PR_COMPRESSION.md)  is a core ability that enables to effectively tackle both short and long PRs.
-- Our JSON prompting strategy enables to have **modular, customizable tools**. For example, the '/review' tool categories can be controlled via the configuration file. Adding additional categories is easy and accessible.
+- Our JSON prompting strategy enables to have **modular, customizable tools**. For example, the '/review' tool categories can be controlled via the [configuration](./CONFIGURATION.md) file. Adding additional categories is easy and accessible.
 - We support **multiple git providers** (GitHub, Gitlab, Bitbucket), and multiple ways to use the tool (CLI, GitHub Action, GitHub App, Docker, ...).
 - We are open-source, and welcome contributions from the community.
 

pr_agent/algo/pr_processing.py

@@ -1,7 +1,10 @@
 from __future__ import annotations
-import traceback
+
+import difflib
 import logging
-from typing import Callable, Tuple
+import re
+import traceback
+from typing import Any, Callable, List, Tuple
 
 from github import RateLimitExceededException
 
@@ -9,9 +12,8 @@ from pr_agent.algo import MAX_TOKENS
 from pr_agent.algo.git_patch_processing import convert_to_hunks_with_lines_numbers, extend_patch, handle_patch_deletions
 from pr_agent.algo.language_handler import sort_files_by_main_languages
 from pr_agent.algo.token_handler import TokenHandler
-from pr_agent.algo.utils import load_large_diff
 from pr_agent.config_loader import get_settings
-from pr_agent.git_providers.git_provider import GitProvider
+from pr_agent.git_providers.git_provider import FilePatchInfo, GitProvider
 
 DELETED_FILES_ = "Deleted files:\n"
 
@@ -46,7 +48,7 @@ def get_pr_diff(git_provider: GitProvider, token_handler: TokenHandler, model: s
         PATCH_EXTRA_LINES = 0
 
     try:
-        diff_files = list(git_provider.get_diff_files())
+        diff_files = git_provider.get_diff_files()
     except RateLimitExceededException as e:
         logging.error(f"Rate limit exceeded for git provider API. original message {e}")
         raise
@@ -98,12 +100,7 @@ def pr_generate_extended_diff(pr_languages: list, token_handler: TokenHandler,
     for lang in pr_languages:
         for file in lang['files']:
             original_file_content_str = file.base_file
-            new_file_content_str = file.head_file
             patch = file.patch
-
-            # handle the case of large patch, that initially was not loaded
-            patch = load_large_diff(file, new_file_content_str, original_file_content_str, patch)
-
             if not patch:
                 continue
 
@@ -161,7 +158,6 @@ def pr_generate_compressed_diff(top_langs: list, token_handler: TokenHandler, mo
         original_file_content_str = file.base_file
         new_file_content_str = file.head_file
         patch = file.patch
-        patch = load_large_diff(file, new_file_content_str, original_file_content_str, patch)
         if not patch:
             continue
 
@@ -224,3 +220,67 @@ async def retry_with_fallback_models(f: Callable):
             logging.warning(f"Failed to generate prediction with {model}: {traceback.format_exc()}")
             if i == len(all_models) - 1:  # If it's the last iteration
                 raise  # Re-raise the last exception
+
+
+def find_line_number_of_relevant_line_in_file(diff_files: List[FilePatchInfo],
+                                              relevant_file: str,
+                                              relevant_line_in_file: str) -> Tuple[int, int]:
+    """
+    Find the line number and absolute position of a relevant line in a file.
+
+    Args:
+        diff_files (List[FilePatchInfo]): A list of FilePatchInfo objects representing the patches of files.
+        relevant_file (str): The name of the file where the relevant line is located.
+        relevant_line_in_file (str): The content of the relevant line.
+
+    Returns:
+        Tuple[int, int]: A tuple containing the line number and absolute position of the relevant line in the file.
+    """
+    position = -1
+    absolute_position = -1
+    re_hunk_header = re.compile(
+        r"^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@[ ]?(.*)")
+
+    for file in diff_files:
+        if file.filename.strip() == relevant_file:
+            patch = file.patch
+            patch_lines = patch.splitlines()
+
+            # try to find the line in the patch using difflib, with some margin of error
+            matches_difflib: list[str | Any] = difflib.get_close_matches(relevant_line_in_file,
+                                                                         patch_lines, n=3, cutoff=0.93)
+            if len(matches_difflib) == 1 and matches_difflib[0].startswith('+'):
+                relevant_line_in_file = matches_difflib[0]
+
+            delta = 0
+            start1, size1, start2, size2 = 0, 0, 0, 0
+            for i, line in enumerate(patch_lines):
+                if line.startswith('@@'):
+                    delta = 0
+                    match = re_hunk_header.match(line)
+                    start1, size1, start2, size2 = map(int, match.groups()[:4])
+                elif not line.startswith('-'):
+                    delta += 1
+
+                if relevant_line_in_file in line and line[0] != '-':
+                    position = i
+                    absolute_position = start2 + delta - 1
+                    break
+
+            if position == -1 and relevant_line_in_file[0] == '+':
+                no_plus_line = relevant_line_in_file[1:].lstrip()
+                for i, line in enumerate(patch_lines):
+                    if line.startswith('@@'):
+                        delta = 0
+                        match = re_hunk_header.match(line)
+                        start1, size1, start2, size2 = map(int, match.groups()[:4])
+                    elif not line.startswith('-'):
+                        delta += 1
+
+                    if no_plus_line in line and line[0] != '-':
+                        # The model might add a '+' to the beginning of the relevant_line_in_file even if originally
+                        # it's a context line
+                        position = i
+                        absolute_position = start2 + delta - 1
+                        break
+    return position, absolute_position

pr_agent/algo/utils.py

@@ -40,7 +40,7 @@ def convert_to_markdown(output_data: dict) -> str:
         "Security concerns": "🔒",
         "General PR suggestions": "💡",
         "Insights from user's answers": "📝",
-        "Code suggestions": "🤖",
+        "Code feedback": "🤖",
     }
 
     for key, value in output_data.items():
@@ -50,12 +50,12 @@ def convert_to_markdown(output_data: dict) -> str:
             markdown_text += f"## {key}\n\n"
             markdown_text += convert_to_markdown(value)
         elif isinstance(value, list):
-            if key.lower() == 'code suggestions':
+            if key.lower() == 'code feedback':
                 markdown_text += "\n"  # just looks nicer with additional line breaks
             emoji = emojis.get(key, "")
             markdown_text += f"- {emoji} **{key}:**\n\n"
             for item in value:
-                if isinstance(item, dict) and key.lower() == 'code suggestions':
+                if isinstance(item, dict) and key.lower() == 'code feedback':
                     markdown_text += parse_code_suggestion(item)
                 elif item:
                     markdown_text += f"  - {item}\n"
@@ -100,7 +100,7 @@ def try_fix_json(review, max_iter=10, code_suggestions=False):
     Args:
     - review: A string containing the JSON message to be fixed.
     - max_iter: An integer representing the maximum number of iterations to try and fix the JSON message.
-    - code_suggestions: A boolean indicating whether to try and fix JSON messages with code suggestions.
+    - code_suggestions: A boolean indicating whether to try and fix JSON messages with code feedback.
 
     Returns:
     - data: A dictionary containing the parsed JSON data.
@@ -108,7 +108,7 @@ def try_fix_json(review, max_iter=10, code_suggestions=False):
     The function attempts to fix broken or incomplete JSON messages by parsing until the last valid code suggestion.
     If the JSON message ends with a closing bracket, the function calls the fix_json_escape_char function to fix the
     message.
-    If code_suggestions is True and the JSON message contains code suggestions, the function tries to fix the JSON
+    If code_suggestions is True and the JSON message contains code feedback, the function tries to fix the JSON
     message by parsing until the last valid code suggestion.
     The function uses regular expressions to find the last occurrence of "}," with any number of whitespaces or
     newlines.
@@ -128,7 +128,8 @@ def try_fix_json(review, max_iter=10, code_suggestions=False):
     else:
         closing_bracket = "]}}"
 
-    if review.rfind("'Code suggestions': [") > 0 or review.rfind('"Code suggestions": [') > 0:
+    if (review.rfind("'Code feedback': [") > 0 or review.rfind('"Code feedback": [') > 0) or \
+            (review.rfind("'Code suggestions': [") > 0 or review.rfind('"Code suggestions": [') > 0) :
         last_code_suggestion_ind = [m.end() for m in re.finditer(r"\}\s*,", review)][-1] - 1
         valid_json = False
         iter_count = 0
@@ -195,35 +196,27 @@ def convert_str_to_datetime(date_str):
     return datetime.strptime(date_str, datetime_format)
 
 
-def load_large_diff(file, new_file_content_str: str, original_file_content_str: str, patch: str) -> str:
+def load_large_diff(filename, new_file_content_str: str, original_file_content_str: str) -> str:
     """
     Generate a patch for a modified file by comparing the original content of the file with the new content provided as
     input.
 
     Args:
-        file: The file object for which the patch needs to be generated.
         new_file_content_str: The new content of the file as a string.
         original_file_content_str: The original content of the file as a string.
-        patch: An optional patch string that can be provided as input.
 
     Returns:
         The generated or provided patch string.
 
     Raises:
         None.
-
-    Additional Information:
-        - If 'patch' is not provided as input, the function generates a patch using the 'difflib' library and returns it
-          as output.
-        - If the 'settings.config.verbosity_level' is greater than or equal to 2, a warning message is logged indicating
-          that the file was modified but no patch was found, and a patch is manually created.
     """
-    if not patch:  # to Do - also add condition for file extension
+    patch = ""
     try:
         diff = difflib.unified_diff(original_file_content_str.splitlines(keepends=True),
                                     new_file_content_str.splitlines(keepends=True))
         if get_settings().config.verbosity_level >= 2:
-                logging.warning(f"File was modified, but no patch was found. Manually creating patch: {file.filename}.")
+            logging.warning(f"File was modified, but no patch was found. Manually creating patch: {filename}.")
         patch = ''.join(diff)
     except Exception:
         pass

pr_agent/git_providers/github_provider.py

@@ -1,4 +1,6 @@
 import logging
+import hashlib
+
 from datetime import datetime
 from typing import Optional, Tuple
 from urllib.parse import urlparse
@@ -10,6 +12,7 @@ from starlette_context import context
 from .git_provider import FilePatchInfo, GitProvider, IncrementalPR
 from ..algo.language_handler import is_valid_file
 from ..algo.utils import load_large_diff
+from ..algo.pr_processing import find_line_number_of_relevant_line_in_file
 from ..config_loader import get_settings
 from ..servers.utils import RateLimitExceeded
 
@@ -27,6 +30,7 @@ class GithubProvider(GitProvider):
         self.pr = None
         self.github_user_id = None
         self.diff_files = None
+        self.git_files = None
         self.incremental = incremental
         if pr_url:
             self.set_pr(pr_url)
@@ -81,40 +85,56 @@ class GithubProvider(GitProvider):
     def get_files(self):
         if self.incremental.is_incremental and self.file_set:
             return self.file_set.values()
-        return self.pr.get_files()
+        if not self.git_files:
+            # bring files from GitHub only once
+            self.git_files = self.pr.get_files()
+        return self.git_files
 
     @retry(exceptions=RateLimitExceeded,
            tries=get_settings().github.ratelimit_retries, delay=2, backoff=2, jitter=(1, 3))
     def get_diff_files(self) -> list[FilePatchInfo]:
+        """
+        Retrieves the list of files that have been modified, added, deleted, or renamed in a pull request in GitHub,
+        along with their content and patch information.
+
+        Returns:
+            diff_files (List[FilePatchInfo]): List of FilePatchInfo objects representing the modified, added, deleted,
+            or renamed files in the merge request.
+        """
         try:
+            if self.diff_files:
+                return self.diff_files
+
             files = self.get_files()
             diff_files = []
+
             for file in files:
-                if is_valid_file(file.filename):
-                    new_file_content_str = self._get_pr_file_content(file, self.pr.head.sha)
+                if not is_valid_file(file.filename):
+                    continue
+
+                new_file_content_str = self._get_pr_file_content(file, self.pr.head.sha)  # communication with GitHub
                 patch = file.patch
+
                 if self.incremental.is_incremental and self.file_set:
-                        original_file_content_str = self._get_pr_file_content(file,
-                                                                              self.incremental.last_seen_commit_sha)
-                        patch = load_large_diff(file,
-                                                new_file_content_str,
-                                                original_file_content_str,
-                                                None)
+                    original_file_content_str = self._get_pr_file_content(file, self.incremental.last_seen_commit_sha)
+                    patch = load_large_diff(file.filename, new_file_content_str, original_file_content_str)
                     self.file_set[file.filename] = patch
                 else:
                     original_file_content_str = self._get_pr_file_content(file, self.pr.base.sha)
+                    if not patch:
+                        patch = load_large_diff(file.filename, new_file_content_str, original_file_content_str)
+
+                diff_files.append(FilePatchInfo(original_file_content_str, new_file_content_str, patch, file.filename))
 
-                    diff_files.append(
-                        FilePatchInfo(original_file_content_str, new_file_content_str, patch, file.filename))
             self.diff_files = diff_files
             return diff_files
+
         except GithubException.RateLimitExceededException as e:
             logging.error(f"Rate limit exceeded for GitHub API. Original message: {e}")
             raise RateLimitExceeded("Rate limit exceeded for GitHub API.") from e
 
     def publish_description(self, pr_title: str, pr_body: str):
         self.pr.edit(title=pr_title, body=pr_body)
-        # self.pr.create_issue_comment(pr_comment)
 
     def publish_comment(self, pr_comment: str, is_temporary: bool = False):
         if is_temporary and not get_settings().config.publish_output_progress:
@@ -131,22 +151,9 @@ class GithubProvider(GitProvider):
     def publish_inline_comment(self, body: str, relevant_file: str, relevant_line_in_file: str):
         self.publish_inline_comments([self.create_inline_comment(body, relevant_file, relevant_line_in_file)])
 
+
     def create_inline_comment(self, body: str, relevant_file: str, relevant_line_in_file: str):
-        self.diff_files = self.diff_files if self.diff_files else self.get_diff_files()
-        position = -1
-        for file in self.diff_files:
-            if file.filename.strip() == relevant_file:
-                patch = file.patch
-                patch_lines = patch.splitlines()
-                for i, line in enumerate(patch_lines):
-                    if relevant_line_in_file in line:
-                        position = i
-                        break
-                    elif relevant_line_in_file[0] == '+' and relevant_line_in_file[1:].lstrip() in line:
-                        # The model often adds a '+' to the beginning of the relevant_line_in_file even if originally
-                        # it's a context line
-                        position = i
-                        break
+        position = find_line_number_of_relevant_line_in_file(self.diff_files, relevant_file.strip('`'), relevant_line_in_file)
         if position == -1:
             if get_settings().config.verbosity_level >= 2:
                 logging.info(f"Could not find position for {relevant_file} {relevant_line_in_file}")
@@ -154,8 +161,6 @@ class GithubProvider(GitProvider):
         else:
             subject_type = "LINE"
         path = relevant_file.strip()
-        # placeholder for future API support (already supported in single inline comment)
-        # return dict(body=body, path=path, position=position, subject_type=subject_type)
         return dict(body=body, path=path, position=position) if subject_type == "LINE" else {}
 
     def publish_inline_comments(self, comments: list[dict]):
@@ -367,3 +372,25 @@ class GithubProvider(GitProvider):
         except:
             commit_messages_str = ""
         return commit_messages_str
+
+    def generate_link_to_relevant_line_number(self, suggestion) -> str:
+        try:
+            relevant_file = suggestion['relevant file']
+            relevant_line_str = suggestion['relevant line']
+            position, absolute_position = find_line_number_of_relevant_line_in_file \
+                (self.diff_files, relevant_file.strip('`'), relevant_line_str)
+
+            if absolute_position != -1:
+                # # link to right file only
+                # link = f"https://github.com/{self.repo}/blob/{self.pr.head.sha}/{relevant_file}" \
+                #        + "#" + f"L{absolute_position}"
+
+                # link to diff
+                sha_file = hashlib.sha256(relevant_file.encode('utf-8')).hexdigest()
+                link = f"https://github.com/{self.repo}/pull/{self.pr_num}/files#diff-{sha_file}R{absolute_position}"
+                return link
+        except Exception as e:
+            if get_settings().config.verbosity_level >= 2:
+                logging.info(f"Failed adding line link, error: {e}")
+
+        return ""

pr_agent/git_providers/gitlab_provider.py

@@ -7,6 +7,7 @@ import gitlab
 from gitlab import GitlabGetError
 
 from ..algo.language_handler import is_valid_file
+from ..algo.utils import load_large_diff
 from ..config_loader import get_settings
 from .git_provider import EDIT_TYPE, FilePatchInfo, GitProvider
 
@@ -30,6 +31,7 @@ class GitLabProvider(GitProvider):
         self.id_mr = None
         self.mr = None
         self.diff_files = None
+        self.git_files = None
         self.temp_comments = []
         self._set_merge_request(merge_request_url)
         self.RE_HUNK_HEADER = re.compile(
@@ -65,19 +67,27 @@ class GitLabProvider(GitProvider):
             return ''
 
     def get_diff_files(self) -> list[FilePatchInfo]:
+        """
+        Retrieves the list of files that have been modified, added, deleted, or renamed in a pull request in GitLab,
+        along with their content and patch information.
+
+        Returns:
+            diff_files (List[FilePatchInfo]): List of FilePatchInfo objects representing the modified, added, deleted,
+            or renamed files in the merge request.
+        """
+
+        if self.diff_files:
+            return self.diff_files
+
         diffs = self.mr.changes()['changes']
         diff_files = []
         for diff in diffs:
             if is_valid_file(diff['new_path']):
-                original_file_content_str = self._get_pr_file_content(diff['old_path'], self.mr.target_branch)
-                new_file_content_str = self._get_pr_file_content(diff['new_path'], self.mr.source_branch)
-                edit_type = EDIT_TYPE.MODIFIED
-                if diff['new_file']:
-                    edit_type = EDIT_TYPE.ADDED
-                elif diff['deleted_file']:
-                    edit_type = EDIT_TYPE.DELETED
-                elif diff['renamed_file']:
-                    edit_type = EDIT_TYPE.RENAMED
+                # original_file_content_str = self._get_pr_file_content(diff['old_path'], self.mr.target_branch)
+                # new_file_content_str = self._get_pr_file_content(diff['new_path'], self.mr.source_branch)
+                original_file_content_str = self._get_pr_file_content(diff['old_path'], self.mr.diff_refs['base_sha'])
+                new_file_content_str = self._get_pr_file_content(diff['new_path'], self.mr.diff_refs['head_sha'])
+
                 try:
                     if isinstance(original_file_content_str, bytes):
                         original_file_content_str = bytes.decode(original_file_content_str, 'utf-8')
@@ -86,15 +96,33 @@ class GitLabProvider(GitProvider):
                 except UnicodeDecodeError:
                     logging.warning(
                         f"Cannot decode file {diff['old_path']} or {diff['new_path']} in merge request {self.id_mr}")
+
+                edit_type = EDIT_TYPE.MODIFIED
+                if diff['new_file']:
+                    edit_type = EDIT_TYPE.ADDED
+                elif diff['deleted_file']:
+                    edit_type = EDIT_TYPE.DELETED
+                elif diff['renamed_file']:
+                    edit_type = EDIT_TYPE.RENAMED
+
+                filename = diff['new_path']
+                patch = diff['diff']
+                if not patch:
+                    patch = load_large_diff(filename, new_file_content_str, original_file_content_str)
+
                 diff_files.append(
-                    FilePatchInfo(original_file_content_str, new_file_content_str, diff['diff'], diff['new_path'],
+                    FilePatchInfo(original_file_content_str, new_file_content_str,
+                                  patch=patch,
+                                  filename=filename,
                                   edit_type=edit_type,
                                   old_filename=None if diff['old_path'] == diff['new_path'] else diff['old_path']))
         self.diff_files = diff_files
         return diff_files
 
     def get_files(self):
-        return [change['new_path'] for change in self.mr.changes()['changes']]
+        if not self.git_files:
+            self.git_files = [change['new_path'] for change in self.mr.changes()['changes']]
+        return self.git_files
 
     def publish_description(self, pr_title: str, pr_body: str):
         try:
@@ -110,7 +138,6 @@ class GitLabProvider(GitProvider):
             self.temp_comments.append(comment)
 
     def publish_inline_comment(self, body: str, relevant_file: str, relevant_line_in_file: str):
-        self.diff_files = self.diff_files if self.diff_files else self.get_diff_files()
         edit_type, found, source_line_no, target_file, target_line_no = self.search_line(relevant_file,
                                                                                          relevant_line_in_file)
         self.send_inline_comment(body, edit_type, found, relevant_file, relevant_line_in_file, source_line_no,
@@ -151,9 +178,9 @@ class GitLabProvider(GitProvider):
                 relevant_lines_start = suggestion['relevant_lines_start']
                 relevant_lines_end = suggestion['relevant_lines_end']
 
-                self.diff_files = self.diff_files if self.diff_files else self.get_diff_files()
+                diff_files = self.get_diff_files()
                 target_file = None
-                for file in self.diff_files:
+                for file in diff_files:
                     if file.filename == relevant_file:
                         if file.filename == relevant_file:
                             target_file = file
@@ -180,7 +207,7 @@ class GitLabProvider(GitProvider):
         target_file = None
 
         edit_type = self.get_edit_type(relevant_line_in_file)
-        for file in self.diff_files:
+        for file in self.get_diff_files():
             if file.filename == relevant_file:
                 edit_type, found, source_line_no, target_file, target_line_no = self.find_in_file(file,
                                                                                                   relevant_line_in_file)

pr_agent/settings/configuration.toml

@@ -13,8 +13,8 @@ require_focused_review=true
 require_score_review=false
 require_tests_review=true
 require_security_review=true
-num_code_suggestions=0
-inline_code_comments = true
+num_code_suggestions=3
+inline_code_comments = false
 ask_and_reflect=false
 extra_instructions = ""
 

pr_agent/settings/pr_reviewer_prompts.toml

@@ -1,9 +1,9 @@
 [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).
-- Provide up to {{ num_code_suggestions }} code suggestions.
 {%- if num_code_suggestions > 0 %}
-- Try to focus on 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.
+- 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 %}
@@ -24,7 +24,7 @@ You must use the following JSON schema to format your answer:
     },
     "Type of PR": {
       "type": "string",
-      "enum": ["Bug fix", "Tests", "Bug fix with tests", "Refactoring", "Enhancement", "Documentation", "Other"]
+      "enum": ["Bug fix", "Tests", "Refactoring", "Enhancement", "Documentation", "Other"]
     },
 {%- if require_score %}
     "Score": {
@@ -47,17 +47,17 @@ You must use the following JSON schema to format your answer:
 {%- if require_focused %}
     "Focused PR": {
       "type": "string",
-      "description": "Is this a focused PR, in the sense that it has a clear and coherent title and description, and all PR code diff changes are properly derived from the title and description? Explain your response."
+      "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 PR suggestions": {
+    "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. Explain your suggestions."
+      "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 suggestions": {
+    "Code feedback": {
       "type": "array",
       "maxItems": {{ num_code_suggestions }},
       "uniqueItems": true,
@@ -66,13 +66,13 @@ You must use the following JSON schema to format your answer:
           "type": "string",
           "description": "the relevant file full path"
         },
-        "suggestion content": {
+        "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 in file": {
+        "relevant line": {
           "type": "string",
-          "description": "an authentic single code line from the PR git diff section, to which the suggestion applies."
+          "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"
         }
       }
     },
@@ -80,8 +80,8 @@ You must use the following JSON schema to format your answer:
 {%- 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 ? explain your answer"
-       ? explain your answer"
+      "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 %}
   }
@@ -109,11 +109,11 @@ Example output:
     {
         "General PR suggestions": "..., `xxx`...",
 {%- if num_code_suggestions > 0 %}
-        "Code suggestions": [
+        "Code feedback": [
             {
                 "relevant file": "directory/xxx.py",
-                "suggestion content": "xxx [important]",
-                "relevant line in file": "xxx",
+                "suggestion": "xxx [important]",
+                "relevant line": "xxx",
             },
             ...
         ]

pr_agent/tools/pr_code_suggestions.py

@@ -58,12 +58,12 @@ class PRCodeSuggestions:
 
     async def _prepare_prediction(self, model: str):
         logging.info('Getting PR diff...')
-        # we are using extended hunk with line numbers for code suggestions
         self.patches_diff = get_pr_diff(self.git_provider,
                                         self.token_handler,
                                         model,
                                         add_line_numbers_to_hunks=True,
                                         disable_extra_lines=True)
+
         logging.info('Getting AI prediction...')
         self.prediction = await self._get_prediction(model)
 

pr_agent/tools/pr_reviewer.py

@@ -7,7 +7,8 @@ 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.pr_processing import get_pr_diff, retry_with_fallback_models, \
+    find_line_number_of_relevant_line_in_file
 from pr_agent.algo.token_handler import TokenHandler
 from pr_agent.algo.utils import convert_to_markdown, try_fix_json
 from pr_agent.config_loader import get_settings
@@ -160,28 +161,38 @@ class PRReviewer:
         the feedback.
         """
         review = self.prediction.strip()
-        print(f"review: {review}")
+
         try:
             data = json.loads(review)
         except json.decoder.JSONDecodeError:
             data = try_fix_json(review)
-        print(f"data: {data}")
+
         # Move 'Security concerns' key to 'PR Analysis' section for better display
-        if 'PR Feedback' in data and 'Security concerns' in data['PR Feedback']:
-            val = data['PR Feedback']['Security concerns']
-            del data['PR Feedback']['Security concerns']
-            data['PR Analysis']['Security concerns'] = val
+        pr_feedback = data.get('PR Feedback', {})
+        security_concerns = pr_feedback.get('Security concerns')
+        if security_concerns:
+            del pr_feedback['Security concerns']
+            data.setdefault('PR Analysis', {})['Security concerns'] = security_concerns
+
+        #
+        if 'Code feedback' in pr_feedback:
+            code_feedback = pr_feedback['Code feedback']
 
             # Filter out code suggestions that can be submitted as inline comments
-        if 'PR Feedback' in data:
-            if get_settings().config.git_provider != 'bitbucket' and get_settings().pr_reviewer.inline_code_comments \
-                    and 'Code suggestions' in data['PR Feedback']:
-                data['PR Feedback']['Code suggestions'] = [
-                    d for d in data['PR Feedback']['Code suggestions']
-                    if any(key not in d for key in ('relevant file', 'relevant line in file', 'suggestion content'))
-                ]
-                if not data['PR Feedback']['Code suggestions']:
-                    del data['PR Feedback']['Code suggestions']
+            if get_settings().pr_reviewer.inline_code_comments:
+                del pr_feedback['Code feedback']
+            else:
+                for suggestion in code_feedback:
+                    relevant_line_str = suggestion['relevant line'].split('\n')[0]
+
+                    # removing '+'
+                    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})"
 
         # Add incremental review section
         if self.incremental.is_incremental:
@@ -210,7 +221,6 @@ class PRReviewer:
         if markdown_text == None or len(markdown_text) == 0:
             markdown_text = review
 
-        print(f"markdown text: {markdown_text}")
         return markdown_text
 
     def _publish_inline_code_comments(self) -> None:
@@ -227,10 +237,10 @@ class PRReviewer:
             data = try_fix_json(review)
 
         comments: List[str] = []
-        for suggestion in data.get('PR Feedback', {}).get('Code suggestions', []):
+        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 in file', '').strip()
-            content = suggestion.get('suggestion content', '')
+            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:
                 logging.info("Skipping inline comment with missing file/line/content")
                 continue

tests/unittest/test_find_line_number_of_relevant_line_in_file.py

@@ -0,0 +1,68 @@
+
+# Generated by CodiumAI
+from pr_agent.git_providers.git_provider import FilePatchInfo
+from pr_agent.algo.pr_processing import find_line_number_of_relevant_line_in_file
+
+
+import pytest
+
+class TestFindLineNumberOfRelevantLineInFile:
+    # Tests that the function returns the correct line number and absolute position when the relevant line is found in the patch
+    def test_relevant_line_found_in_patch(self):
+        diff_files = [
+            FilePatchInfo(base_file='file1', head_file='file1', patch='@@ -1,1 +1,2 @@\n-line1\n+line2\n+relevant_line\n', filename='file1')
+        ]
+        relevant_file = 'file1'
+        relevant_line_in_file = 'relevant_line'
+        expected = (3, 2) # (position in patch, absolute_position in new file)
+        assert find_line_number_of_relevant_line_in_file(diff_files, relevant_file, relevant_line_in_file) == expected
+
+    # Tests that the function returns the correct line number and absolute position when a similar line is found using difflib
+    def test_similar_line_found_using_difflib(self):
+        diff_files = [
+            FilePatchInfo(base_file='file1', head_file='file1', patch='@@ -1,1 +1,2 @@\n-line1\n+relevant_line in file similar match\n', filename='file1')
+        ]
+        relevant_file = 'file1'
+        relevant_line_in_file = '+relevant_line in file similar match ' # note the space at the end. This is to simulate a similar line found using difflib
+        expected = (2, 1)
+        assert find_line_number_of_relevant_line_in_file(diff_files, relevant_file, relevant_line_in_file) == expected
+
+    # Tests that the function returns (-1, -1) when the relevant line is not found in the patch and no similar line is found using difflib
+    def test_relevant_line_not_found(self):
+        diff_files = [
+            FilePatchInfo(base_file='file1', head_file='file1', patch='@@ -1,1 +1,2 @@\n-line1\n+relevant_line\n', filename='file1')
+        ]
+        relevant_file = 'file1'
+        relevant_line_in_file = 'not_found'
+        expected = (-1, -1)
+        assert find_line_number_of_relevant_line_in_file(diff_files, relevant_file, relevant_line_in_file) == expected
+
+    # Tests that the function returns (-1, -1) when the relevant file is not found in any of the patches
+    def test_relevant_file_not_found(self):
+        diff_files = [
+            FilePatchInfo(base_file='file1', head_file='file1', patch='@@ -1,1 +1,2 @@\n-line1\n+relevant_line\n', filename='file2')
+        ]
+        relevant_file = 'file1'
+        relevant_line_in_file = 'relevant_line'
+        expected = (-1, -1)
+        assert find_line_number_of_relevant_line_in_file(diff_files, relevant_file, relevant_line_in_file) == expected
+
+    # Tests that the function returns (-1, -1) when the relevant_line_in_file is an empty string
+    def test_empty_relevant_line(self):
+        diff_files = [
+            FilePatchInfo(base_file='file1', head_file='file1', patch='@@ -1,1 +1,2 @@\n-line1\n+relevant_line\n', filename='file1')
+        ]
+        relevant_file = 'file1'
+        relevant_line_in_file = ''
+        expected = (0, 0)
+        assert find_line_number_of_relevant_line_in_file(diff_files, relevant_file, relevant_line_in_file) == expected
+
+    # Tests that the function returns (-1, -1) when the relevant_line_in_file is found in the patch but it is a deleted line
+    def test_relevant_line_found_but_deleted(self):
+        diff_files = [
+            FilePatchInfo(base_file='file1', head_file='file1', patch='@@ -1,2 +1,1 @@\n-line1\n-relevant_line\n', filename='file1')
+        ]
+        relevant_file = 'file1'
+        relevant_line_in_file = 'relevant_line'
+        expected = (-1, -1)
+        assert find_line_number_of_relevant_line_in_file(diff_files, relevant_file, relevant_line_in_file) == expected