-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add review risk assessment and optional repository review rules #2391
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
base: main
Are you sure you want to change the base?
Changes from 16 commits
40e5188
104a696
5a74da3
fc8ee1d
d8cbb89
e237f87
13136c6
03e0a0d
2d099fa
71011b3
666a265
8c93ceb
15d8940
20a256d
b71e7be
b97e833
983d227
9298146
20673da
d2f6982
6020941
ca9ab5e
c133868
b2b3f6a
fecf295
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,6 +81,12 @@ require_security_review=true | |
| require_estimate_contribution_time_cost=false | ||
| require_todo_scan=false | ||
| require_ticket_analysis_review=true | ||
| require_risk_assessment=false | ||
| require_merge_recommendation=false | ||
| require_priority_files=false | ||
| enable_review_rules=true | ||
| review_rules_paths=[".pr_agent/review_rules.md",".github/review_rules.md","docs/review_rules.md"] | ||
| max_review_rules_tokens=1200 | ||
|
Comment on lines
+84
to
+89
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. .pr_agent.toml missing new keys New /review configuration knobs were added to pr_agent/settings/configuration.toml, but the mirrored root .pr_agent.toml was not updated to include these options, risking divergence and user confusion about available/default behavior. This violates the mirror-sync requirement when behavior/config surface area changes. Agent Prompt
|
||
| # general options | ||
| publish_output_no_suggestions=true # Set to "false" if you only need the reviewer's remarks (not labels, not "security audit", etc.) and want to avoid noisy "No major issues detected" comments. | ||
| persistent_comment=true | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import copy | ||
| import datetime | ||
| import traceback | ||
|
|
||
| from collections import OrderedDict | ||
|
Comment on lines
+4
to
5
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. Extra blank lines in imports The PR introduces three consecutive blank lines within the import block, which is likely to fail Ruff/PEP8 blank-line rules and create unnecessary diff noise. This can break linting CI and violates the repository formatting conventions. Agent Prompt
|
||
| from functools import partial | ||
| from typing import List, Tuple | ||
|
|
@@ -13,7 +14,7 @@ | |
| get_pr_diff, | ||
| retry_with_fallback_models) | ||
| from pr_agent.algo.token_handler import TokenHandler | ||
| from pr_agent.algo.utils import (ModelType, PRReviewHeader, | ||
| from pr_agent.algo.utils import (ModelType, PRReviewHeader, clip_tokens, | ||
| convert_to_markdown_v2, github_action_output, | ||
| load_yaml, show_relevant_configurations) | ||
| from pr_agent.config_loader import get_settings | ||
|
|
@@ -66,6 +67,8 @@ def __init__(self, pr_url: str, is_answer: bool = False, is_auto: bool = False, | |
| answer_str, question_str = self._get_user_answers() | ||
| self.pr_description, self.pr_description_files = ( | ||
| self.git_provider.get_pr_description(split_changes_walkthrough=True)) | ||
| self.review_rules = self._get_review_rules() | ||
|
|
||
| if (self.pr_description_files and get_settings().get("config.is_auto_command", False) and | ||
| get_settings().get("config.enable_ai_metadata", False)): | ||
| add_ai_metadata_to_diff_files(self.git_provider, self.pr_description_files) | ||
|
|
@@ -85,6 +88,9 @@ def __init__(self, pr_url: str, is_answer: bool = False, is_auto: bool = False, | |
| "require_score": get_settings().pr_reviewer.require_score_review, | ||
| "require_tests": get_settings().pr_reviewer.require_tests_review, | ||
| "require_estimate_effort_to_review": get_settings().pr_reviewer.require_estimate_effort_to_review, | ||
| "require_risk_assessment": get_settings().pr_reviewer.get("require_risk_assessment", False), | ||
| "require_merge_recommendation": get_settings().pr_reviewer.get("require_merge_recommendation", False), | ||
| "require_priority_files": get_settings().pr_reviewer.get("require_priority_files", False), | ||
| "require_estimate_contribution_time_cost": get_settings().pr_reviewer.require_estimate_contribution_time_cost, | ||
| 'require_can_be_split_review': get_settings().pr_reviewer.require_can_be_split_review, | ||
| 'require_security_review': get_settings().pr_reviewer.require_security_review, | ||
|
|
@@ -99,6 +105,8 @@ def __init__(self, pr_url: str, is_answer: bool = False, is_auto: bool = False, | |
| "related_tickets": get_settings().get('related_tickets', []), | ||
| 'duplicate_prompt_examples': get_settings().config.get('duplicate_prompt_examples', False), | ||
| "date": datetime.datetime.now().strftime('%Y-%m-%d'), | ||
| "review_rules": self.review_rules, | ||
|
|
||
| } | ||
|
|
||
| self.token_handler = TokenHandler( | ||
|
|
@@ -117,6 +125,70 @@ def parse_incremental(self, args: List[str]): | |
| incremental = IncrementalPR(is_incremental) | ||
| return incremental | ||
|
|
||
| def _get_review_rules(self) -> str: | ||
| if not get_settings().pr_reviewer.get("enable_review_rules", False): | ||
| return "" | ||
|
|
||
| rule_paths = get_settings().pr_reviewer.get("review_rules_paths", []) or [] | ||
| if isinstance(rule_paths, str): | ||
| rule_paths = [rule_paths] | ||
| elif isinstance(rule_paths, (list, tuple)): | ||
| rule_paths = [ | ||
| str(rule_path).strip() | ||
| for rule_path in rule_paths | ||
| if str(rule_path).strip() | ||
| ] | ||
| else: | ||
| get_logger().warning( | ||
| "Invalid review_rules_paths value; expected string or list of strings", | ||
| artifacts={"review_rules_paths": rule_paths}, | ||
| ) | ||
| rule_paths = [] | ||
|
|
||
|
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
|
||
| base = getattr(getattr(self.git_provider, "pr", None), "base", None) | ||
| ref = ( | ||
| getattr(base, "sha", None) | ||
| or getattr(base, "ref", None) | ||
| or getattr(self.git_provider, "base_sha", None) | ||
| or getattr(self.git_provider, "base_ref", None) | ||
| or getattr(getattr(self.git_provider, "mr", None), "target_branch", None) | ||
| ) | ||
| if not ref: | ||
| get_logger().warning("Could not resolve a trusted base ref for review rules") | ||
| return "" | ||
|
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
Outdated
|
||
| loaded_rules = [] | ||
| loaded_rule_paths = [] | ||
|
|
||
| for rule_path in rule_paths: | ||
| try: | ||
| rule_content = self.git_provider.get_pr_file_content(rule_path, ref) | ||
| except Exception: | ||
| continue | ||
|
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
|
||
|
|
||
| if rule_content and rule_content.strip(): | ||
| loaded_rule_paths.append(rule_path) | ||
| loaded_rules.append(f"File: `{rule_path}`\n{rule_content.strip()}") | ||
|
|
||
| if not loaded_rules: | ||
| get_logger().info("No review rules file found for this PR") | ||
| return "" | ||
|
|
||
| review_rules = "\n\n---\n\n".join(loaded_rules) | ||
| max_tokens = get_settings().pr_reviewer.get("max_review_rules_tokens", 0) | ||
| try: | ||
| max_tokens = int(max_tokens) | ||
| except (TypeError, ValueError): | ||
| get_logger().warning( | ||
| "Invalid max_review_rules_tokens value; skipping token clipping", | ||
| artifacts={"max_review_rules_tokens": max_tokens}, | ||
| ) | ||
|
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
|
||
| max_tokens = 0 | ||
|
|
||
| if max_tokens > 0: | ||
| review_rules = clip_tokens(review_rules, max_tokens) | ||
| get_logger().info("Loaded review rules for this PR", artifacts={"rule_files": loaded_rule_paths}) | ||
|
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
|
||
| return review_rules | ||
|
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
|
||
|
|
||
| async def run(self) -> None: | ||
| try: | ||
| if not self.git_provider.get_files(): | ||
|
|
@@ -173,7 +245,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=f"{PRReviewHeader.REGULAR.value} 🔍", | ||
| initial_header=f"{PRReviewHeader.REGULAR.value}", | ||
| update_header=True, | ||
| final_update_message=final_update_message, ) | ||
|
Comment on lines
238
to
243
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Persistent header string mismatch PRReviewer now uses an initial_header without the "🔍" suffix while convert_to_markdown_v2() still emits a header that includes it. In persistent-comment mode this causes publish_persistent_comment_full() to replace only the prefix and leave a stray "🔍" line in the updated comment body. Agent Prompt
|
||
| else: | ||
|
|
@@ -231,17 +303,32 @@ def _prepare_pr_review(self) -> str: | |
| Prepare the PR review by processing the AI prediction and generating a markdown-formatted text that summarizes | ||
| the feedback. | ||
| """ | ||
| first_key = 'review' | ||
| last_key = 'security_concerns' | ||
| data = load_yaml(self.prediction.strip(), | ||
| keys_fix_yaml=["ticket_compliance_check", "estimated_effort_to_review_[1-5]:", "security_concerns:", "key_issues_to_review:", | ||
| "relevant_file:", "relevant_line:", "suggestion:"], | ||
| first_key=first_key, last_key=last_key) | ||
| first_key = "review" | ||
| last_key = "security_concerns" | ||
| data = load_yaml( | ||
| self.prediction.strip(), | ||
| keys_fix_yaml=[ | ||
| "ticket_compliance_check:", | ||
| "estimated_effort_to_review_[1-5]:", | ||
| "risk_level:", | ||
| "merge_recommendation:", | ||
| "security_concerns:", | ||
| "key_issues_to_review:", | ||
| "relevant_file:", | ||
| "relevant_line:", | ||
| "suggestion:", | ||
| ], | ||
| first_key=first_key, | ||
| last_key=last_key, | ||
| ) | ||
| github_action_output(data, 'review') | ||
|
|
||
| if 'review' not in data: | ||
| get_logger().exception("Failed to parse review data", artifact={"data": data}) | ||
| return "" | ||
| get_logger().info(f"Risk level: {data.get('review', {}).get('risk_level')}") | ||
| get_logger().info(f"Merge recommendation: {data.get('review', {}).get('merge_recommendation')}") | ||
| get_logger().info(f"Priority files: {data.get('review', {}).get('review_priority_files')}") | ||
|
|
||
| # move data['review'] 'key_issues_to_review' key to the end of the dictionary | ||
| if 'key_issues_to_review' in data['review']: | ||
|
|
@@ -273,6 +360,7 @@ def _prepare_pr_review(self) -> str: | |
| # Add custom labels from the review prediction (effort, security) | ||
| self.set_review_labels(data) | ||
|
|
||
|
|
||
| if markdown_text == None or len(markdown_text) == 0: | ||
| markdown_text = "" | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3. Headings break gfm table
🐞 Bug≡ CorrectnessAgent Prompt
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools